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: don't bundle dependencies #8523

Closed
wants to merge 7 commits into from

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Apr 20, 2023

The ecosystem generally expects that dependencies are external. E.g. if there's a security issue, it will get flagged by npm audit if external. If it's bundled then we need to make sure we're raising a security vulnerability against Svelte which bundles a vulnerable version of a library and we're not doing that, which means that all security vulnerabilities in dependencies go unflagged.

This may result in more being downloaded on average. However, many of our dependencies like magic-string, @jridgewell/sourcemap-codec, etc. are already used by other projects like Rollup, svelte-preprocess, etc. For npm users, this will avoid the download cost if the dependency is already used by the project. For users using package managers like pnpm, this is an even bigger win as those dependencies won't need to be downloaded if they are used by any other project you've already installed.

We also avoid generating source maps for these packages when we avoid bundling, which will really shrink the size of the Svelte package. For packages like periscopic which distribute unbundled source, this is a big win as they don't need source maps at all. For packages which have sourcemaps of their own, we don't get that savings, but we could probably convert some of the libraries to this lighter-weight way of being distributed especially if they're ones we control.

@vercel
Copy link

vercel bot commented Apr 20, 2023

@benmccann is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@benmccann benmccann added this to the 4.x milestone Apr 20, 2023
@@ -98,26 +98,42 @@ export default [
}
]
},
/* compiler.js */
// compiler.js - ESM version
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if we didn't need the ESM version at all - just unbundled. Is there something stopping is from doing that?
If there is then that begs the question if it's worthwhile to even do the TS->JSdoc conversion.

Copy link
Member Author

Choose a reason for hiding this comment

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

PR for unbundling here: #8613

@Conduitry
Copy link
Member

Having the UMD version for the REPL bundle specific versions of dependencies - but having the published package used for consumption everywhere else use prod dependencies with ranges - makes me uncomfortable. Has this been discussed? Is there any way around this? If we still need UMD builds for the REPL, can they live somewhere else besides in the same main package?

@dummdidumm
Copy link
Member

This has not been discussed, no, but you make a good point.
What do you mean by "besides the same main package"? A new package like @sveltejs/svelte-precompiled?
What about the CJS output?

@benmccann
Copy link
Member Author

Why is it that we need UMD builds for the REPL? Can it not use ESM? Do we need a bundled version so that it's smaller?

Perhaps we should have a +server.js for the REPL which bundles it at request time and caches the result so that most requests are fast. Then we could have smaller downloads for all our users since they don't need us distributing UMD builds in the npm package.

@dummdidumm
Copy link
Member

How would bundling at request time look like? And how would caching look like? svelte/[version]/+server.js, use ISR to cache, use rollup web build to bundle? Will downloading from npm on the server on the fly succeed, given that there may be some fs access or other funky stuff in some of the dependencies which might not work in a deployed environment?

@benmccann
Copy link
Member Author

svelte/[version]/+server.js

sure. doesn't matter what the exact URL is, but that seems fine to me

use ISR to cache

I'm not that familiar with ISR, but if it works then sure. Any file system or object storage would also work

use rollup web build to bundle?

Yes

Will downloading from npm on the server on the fly succeed, given that there may be some fs access or other funky stuff in some of the dependencies which might not work in a deployed environment?

I don't understand. We're not running the dependencies on the server (just bundling them) so why would we care if they access the fs? Also, if they did do that, wouldn't we already be failing at runtime today?

@PuruVJ
Copy link
Collaborator

PuruVJ commented May 9, 2023

I would not go with +server.js approach as it would bind the REPL with svelte.dev, and not be a standalone component at all.

Precompiled seems like a good idea @sveltejs/precompiled. It does have the issue where a dependency bundled in it may be different version than the esm version for users, but will it cause that many issues if we set a sensible enough semver range for svelte's dependencies?

@MrHBS
Copy link

MrHBS commented May 21, 2023

If no bundling is done, why not use tsc directly and get rid of rollup?

@benmccann benmccann mentioned this pull request May 23, 2023
5 tasks
package.json Outdated Show resolved Hide resolved
@dummdidumm dummdidumm changed the base branch from version-4 to unbundled-esm May 24, 2023 11:06
@dummdidumm
Copy link
Member

Closing in favor of #8613, I merged the package.json changes into it.

@dummdidumm dummdidumm closed this May 24, 2023
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.

None yet

6 participants