Skip to content

Commit

Permalink
Fix request route config deprecation warnings (#409)
Browse files Browse the repository at this point in the history 
* fix: request route config deprecation

* Add version constraint

* Update version requirement

Co-authored-by: KaKa <23028015+climba03003@users.noreply.github.com>

* Fix versioning

---------

Co-authored-by: KaKa <23028015+climba03003@users.noreply.github.com>
  • Loading branch information
@TTPO100AJIEX @climba03003
TTPO100AJIEX and climba03003 authored Sep 13, 2023
1 parent 0d4f80b commit e0a816e
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ async function fastifyStatic (fastify, opts) {
}

function serveFileHandler (req, reply) {
const routeConfig = req.routeConfig
const routeConfig = req.routeOptions.config
pumpSendToReply(req, reply, routeConfig.file, routeConfig.rootPath)
}
}
Expand Down Expand Up @@ -552,7 +552,7 @@ function getRedirectUrl (url) {
}

module.exports = fp(fastifyStatic, {
fastify: '4.x',
fastify: '^4.23.0',
name: '@fastify/static'
})
module.exports.default = fastifyStatic
Expand Down

13 comments on commit e0a816e

@yogeshwar607
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@climba03003 @fastify/static is used in fastify-swagger-ui as dependency where it gets bumped up. In today release we added fastify minimum version check here which is breaking for older fastify version >=4.22.x . So either we fix fastify-swagger-ui to not use bumped version or we make sure @fastify/static works for both cases . Let me know which approach we want to go ahead . Happy to help

@Uzlopak
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please open an issue in fastify-swagger-ui

@Eomm
Copy link
Member

@Eomm Eomm commented on e0a816e Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you update a fastify's module and not the fastify core itself?

@DanelGorgan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also breaking @nestjs/swagger (and I believe everything that is using this package). Why not keeping the backwards compatibility for fastify 4.22.x?

@Uzlopak
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not breaking. You just get one time a warning.

@DanelGorgan
Copy link

@DanelGorgan DanelGorgan commented on e0a816e Sep 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick reply. Well, then it's just the nest app which is actually failing for this reason. There is already an ongoing discussion there.

@Uzlopak
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a link?

@hyoretsu
Copy link

@hyoretsu hyoretsu commented on e0a816e Sep 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seriously, just why? My perfectly fine Nest.js backend, IN PRODUCTION, started to crash after adding a new commit, just because of this. And for what, just to hide some warnings? Shouldn't a breaking change switch major versions?

It invalidates what you have defined as dependencies:
image
^4.x is not supported anymore.

"Open an issue on the affected package"
So because of a minor version bump in a package, I'm forced to do a major version bump of another?

Also, wouldn't simply changing YOUR package.json fix it?

@hyoretsu
Copy link

@hyoretsu hyoretsu commented on e0a816e Sep 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Uzlopak In your side, it probably didn't affect anything, but in many users' it did.

I'll have to ignore this update, or else I'm simply unable to bring my application back up without waiting for an update on @nestjs/platform-fastify's side, since they're independent packages and not even installing fastify@^4.23.0 worked.

I'm not an expert in publishing packages, but I don't think this is a good thing to let happen, or force users to do.

@Uzlopak
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear diary,

today the user @hyoretsu wrote a comment about being frustrated about our code. I could understand his concerns, but
I also got frustrated. fastify modules are open source and we try our best to provide good and reliable code.
Of course he was correct with his assessment, but why did he not wrote a code change to fix the issue?
He is also a prorgammer and a colleague. We should respect us like colleagues.

Good thing is that @climba03003 provided a PR #410 to fix this issue. We will fast-track the PR and solve the regression.

Anyhow, it was a good day.

@hyoretsu
Copy link

@hyoretsu hyoretsu commented on e0a816e Sep 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because as I said, I'm not an expert in publishing packages, having only done so a few times. I have never wrote a plugin, let alone for Fastify, and have actually only ever used it with Nest.js. I'm not the one who created something thousands of people depend on, nor the one who approved the PR that messed things up.

What I did do was, as an end-user, depend on his package and have his application broken by it. If it were at peak usage, this would've been a catastrophe.

As someone who also provides something other people depend on (apps), I myself am pressured if something goes wrong since I'm the one responsible for it. And if they get mad that something went wrong, rightfully so, because of something that I messed up, that's entirely on me.

Users complain when things don't go as expected.

@Uzlopak
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

si tacuisses, philosophus mansisses

@Eomm
Copy link
Member

@Eomm Eomm commented on e0a816e Sep 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seriously, just why? My perfectly fine Nest.js backend, IN PRODUCTION, started to crash after adding a new commit, just because of this

Today, you have learned that you should use the package-lock.json in any PRODUCTION application.
https://docs.npmjs.com/cli/v10/configuring-npm/package-lock-json

Running npm install in prod is a suicide since forever.
Use npm ci instead: https://docs.npmjs.com/cli/v10/commands/npm-ci

6.11.2 released.

I hope you will thank @climba03003 for the fix and the team for the review.

if it were at peak usage, this would've been a catastrophe.

PS: be sure to read the MIT LICENSE:
https://github.com/fastify/fastify-static/blob/master/LICENSE

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.

I'll repeat: package-lock is the way

PPS:

There are funny things in the world:

So, don't rely

Please sign in to comment.