Skip to content

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

Merged
merged 1 commit into from
Dec 2, 2022

Conversation

mydea
Copy link
Member

@mydea mydea commented Dec 1, 2022

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. If there is a dependency that you are referencing in a bundle, but it is not added as dependencies, you have to add it as peerDependencies.

I tried to locally yarn clean && yarn build and checked all the build/esm outputs, and there was no unexpected node_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.

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.
@mydea mydea self-assigned this Dec 1, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.57 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 60.59 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.36 KB (+0.02% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 54.15 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.12 KB (0%)
@sentry/browser - Webpack (minified) 65.75 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.14 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 45.99 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.45 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.88 KB (+0.02% 🔺)
@sentry/replay index.js 37.2 KB (0%)
@sentry/replay - Webpack (gzipped + minified) 37.17 KB (0%)
@sentry/replay - Webpack (minified) 124.99 KB (0%)

@mydea mydea requested review from lobsterkatie and Lms24 December 1, 2022 09:08
Copy link
Member

@Lms24 Lms24 left a 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 neither dependency nor peerDependency, 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 as dependencies, you have to add it as peerDependencies.

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.

@mydea
Copy link
Member Author

mydea commented Dec 1, 2022

Yeah, agreed!

Regarding how that works with rrweb, I tried it, and basically it puts a node_modules/rrweb folder into the build dir. Then, the regular tree shaking etc. happens from there!

@Lms24
Copy link
Member

Lms24 commented Dec 1, 2022

into the build dir

Just to confirm, into build/npm/node_modules?

@mydea
Copy link
Member Author

mydea commented Dec 1, 2022

Just to confirm, into build/npm/node_modules?

Yes, exactly - this is how it looks like for me locally (with rrweb being moved to devDeps):

Screenshot 2022-12-01 at 13 26 34

@Lms24
Copy link
Member

Lms24 commented Dec 1, 2022

Awesome!

@mydea mydea merged commit 91c8f37 into master Dec 2, 2022
@mydea mydea deleted the fn/do-not-exclude-devDeps branch December 2, 2022 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants