Skip to content
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

Merged
merged 10 commits into from
Apr 27, 2023
Merged

chore: update dependencies #9761

merged 10 commits into from
Apr 27, 2023

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Apr 24, 2023

fixes #9549
fixes #9427

@changeset-bot
Copy link

changeset-bot bot commented Apr 24, 2023

🦋 Changeset detected

Latest commit: d032067

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@sveltejs/adapter-cloudflare Patch
@sveltejs/adapter-cloudflare-workers Patch
@sveltejs/adapter-netlify Patch
@sveltejs/adapter-node Patch
@sveltejs/adapter-vercel Patch

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

Copy link
Member

@Conduitry Conduitry left a 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.

@benmccann
Copy link
Member Author

Oh yeah, good catch. Though it makes me wonder why it's in devDependencies vs dependencies. If there's ever a security issue in any of these libraries users aren't going to be notified by npm audit. There's another bundling step with rollup in the adapter, so presumably the adapter could bundle the dependency in rather than us doing it before distributing adapter-node.

@Conduitry
Copy link
Member

It should be theoretically possible, but the module resolution might be tricky, since then sirv would be a prod dependency of the Node adapter, and not a dev dependency of the user application. The handler.js file that gets copied into the user app would need to be able to access an indirect dependency when the app is getting bundled.

@benmccann
Copy link
Member Author

I don't get why that makes things tricky. adapter-node will invoke Rollup bundling everything except what the user puts in dependencies. Since that's not in dependencies it should get bundled.

I tested it locally and it seemed to work, so I updated this PR to make that change.

Copy link
Member

@Conduitry Conduitry left a 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.

@gtm-nayan
Copy link
Contributor

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.

@itssumitrai
Copy link

Yes please, a swift fix for now is needed 🙏 Memory leak issue affects production applications.

@benmccann
Copy link
Member Author

I've reverted the bundling change so this is now a straight upgrade of sirv

Copy link
Member

@Conduitry Conduitry left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sveltekit demo project deployed to Heroku gives HTTP 416, when using Linkedin Post Inspector Memory leak?
5 participants