-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
build: Do not exclude devDependencies
from rollup build
#6358
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
Conversation
Currently, our rollup config adds all `dependencies`, `devDependencies` and `peerDependencies` to the `external` array, meaning they will not be inlined into the build. However, that makes it impossible to _selectively_ inline files. For example, in replay we'd like to inline `rrweb` into the bundle. What makes this even more tricky is that because of the nature of `deepmerge`, which we use to merge custom config into the default one, it is actually impossible to overwrite this, as a custom `external` array will not overwrite the default one, but be added to it. This change removes `devDependencies` from the externals. IMHO that is otherwise bound to break anyhow - as if the dependency is neither `dependency` nor `peerDependency`, and you import it (but not bundle it), stuff might break anyhow.
size-limit report 📦
|
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.
This change removes
devDependencies
from the externals. IMHO that is otherwise bound to break anyhow - as if the dependency is neitherdependency
norpeerDependency
, and you import it (but not bundle it), stuff might break anyhow. If there is a dependency that you are referencing in a bundle, but it is not added asdependencies
, you have to add it aspeerDependencies
.
That's a good point and I generally agree with it. As we had broken releases in the past because of build changes, I'd be curious to get a second opinion here from @lobsterkatie as you're the expert on build-related things.
I'm generally curious if this combination of inlining rrweb and keeping our transpiled JS in separate files works. In case it doesn't we can still try bundling our JS into one file, like Replay did originally.
Yeah, agreed! Regarding how that works with rrweb, I tried it, and basically it puts a |
Just to confirm, into |
Awesome! |
Currently, our rollup config adds all
dependencies
,devDependencies
andpeerDependencies
to theexternal
array, meaning they will not be inlined into the build.However, that makes it impossible to selectively inline files. For example, in replay we'd like to inline
rrweb
into the bundle.What makes this even more tricky is that because of the nature of
deepmerge
, which we use to merge custom config into the default one, it is actually impossible to overwrite this, as a customexternal
array will not overwrite the default one, but be added to it.This change removes
devDependencies
from the externals. IMHO that is otherwise bound to break anyhow - as if the dependency is neitherdependency
norpeerDependency
, and you import it (but not bundle it), stuff might break anyhow. If there is a dependency that you are referencing in a bundle, but it is not added asdependencies
, you have to add it aspeerDependencies
.I tried to locally
yarn clean && yarn build
and checked all thebuild/esm
outputs, and there was no unexpectednode_modules
stuff to be found there. We generally use few dependencies, so as far as I can tell this has no actual impact on current output.