-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: update dependencies #9761
Conversation
🦋 Changeset detectedLatest commit: d032067 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in #9549 (comment) we do need to upgrade this ourselves (including for the Node adapter) and release a new version, as sirv is bundled.
Oh yeah, good catch. Though it makes me wonder why it's in |
It should be theoretically possible, but the module resolution might be tricky, since then |
I don't get why that makes things tricky. I tested it locally and it seemed to work, so I updated this PR to make that change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this PR works because the generated files/handler.js
that gets copied into the user's app still has sirv etc. bundled, even though they're now prod dependencies. Take a look at that file after building adapter-node locally, and then take a look at its rollup.config.js
.
I still think we should run these changes by Rich. My feeling is that we should just release another version of adapter-node with the bundled version of sirv upgraded, and then we can come back with the required degree of care to stop bundling some of these dependencies if that's something we want to do.
edit: It would also be good if we were able to stop bundling @sveltejs/kit/node/polyfills
because the way that works is very confusing. The version of undici we're using is a prod dependency of @sveltejs/kit
, but users won't actually be using that version of it. They'll be using the version bundled in the released @sveltejs/adapter-node
(or @sveltejs/adapter-netlify
), which have devdeps on the workspace dependency of Kit, and changesets will have no idea that there are pending changes of these packages when we do bump the dep of undici in Kit. I reopened #9427 for this reason.
The memory leak issue is waiting a new version for adapter-node as well so I agree with Conduitry, it'd be better to get a version out with minimal changes and then think about this. I was experimenting with getting rid of the build step entirely for the node adapter earlier, branch here. Moving them to deps did work with the linked package but not sure if the published version will behave differently. |
Yes please, a swift fix for now is needed 🙏 Memory leak issue affects production applications. |
I've reverted the bundling change so this is now a straight upgrade of sirv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changeset needs upgrading.
We should also try to figure out what version of undici is bundled in the last-released version of the Node adapter and mention the upgrade in the Node adapter changeset.
We might also want to take a look at the Netlify adapter, because I believe it also bundles undici from @sveltejs/kit/node/polyfills
, and take a look to make sure none of the other adapters have the same bundling thing happening.
fixes #9549
fixes #9427