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

build(replay): Rename node_modules directory inside build output directories #6757

Closed
wants to merge 3 commits into from

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jan 12, 2023

As reported in #6690, apparently some bundlers and plugins seem to have problems when JS modules import from other modules under a node_modules directory. After we decided to vendor rrweb to patch it, we have such a directory inside our build/npm/(esm|cjs)/node_modules/rrweb/.... This PR adds the rollup-plugin-rename-node-modules plugin which takes care of renaming such paths during the rollup build.

With this change, rrweb can now be found under build/npm/(esm|cjs)/ext/rrweb/.... Let's see if this fixes the bug reports.

Tested this change locally with a plain-js test app (npm package as well as minified cdn bundles).

Possibly fixes #6690

@Lms24 Lms24 requested review from mydea and billyvg January 12, 2023 15:34
@lforst
Copy link
Member

lforst commented Jan 12, 2023

We should check if this influences our source maps in some way. Looking at the packages' code it does some non-trivial stuff.

If, for some reason this change has issues we could try the following: Bundle the rrweb dependency in a previous rollup step with preserveModules: false, gitignore the output, and mark rrweb as external in the bundling of the replay package.

@Lms24
Copy link
Member Author

Lms24 commented Jan 12, 2023

We should check if this influences our source maps in some way. Looking at the packages' code it does some non-trivial stuff

Afaik, it uses MagicString to adjust source maps

There is another alternative to this: We were just discussing releasing rrweb from our getsentry/rrweb fork which would make patching the actual rrweb package unnecessary. Hence we could stop pulling in the files which would also remove said node_modules directory. Just wanted to get this in as I'm guessing, we still need some time to actually release those packages

@github-actions
Copy link
Contributor

github-actions bot commented Jan 12, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.84 KB (0%)
@sentry/browser - ES5 CDN Bundle (minified) 61.46 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.62 KB (+0.02% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 55 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.38 KB (0%)
@sentry/browser - Webpack (minified) 66.55 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.4 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.63 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.82 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.25 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 43.26 KB (-0.01% 🔽)
@sentry/replay - Webpack (gzipped + minified) 38.5 KB (0%)

@Lms24
Copy link
Member Author

Lms24 commented Jan 12, 2023

Lol funny observation: rrweb also uses this very plugin to rename vendored dependencies: https://github.com/getsentry/rrweb/blob/f1d2080a649875f054238cb54f935f1a2b36d5c2/packages/rrweb/rollup.config.js#L177

Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

I think this is a great short term solution. Mid term we should explore publishing our fork, that would streamline this - but for now, seems like an easy fix!

@lforst
Copy link
Member

lforst commented Jan 12, 2023

I also like the idea of publishing the fork!

@Lms24
Copy link
Member Author

Lms24 commented Jan 12, 2023

@lforst your concern turned out to be valid: With this plugin, the generated source maps seem to get majorly destroyed: The maps dont contain any sources or sourcesContent information anymore, which they did before. However, I can simply disable changing source maps within the plugin. Not sure if I'm missing something crucially here but I think this is okay, as the only thing that changes in our transpiled files is the path inside import/require statements (i.e. s/node_modules/ext/). Not 100% sure about this, though

@mydea
Copy link
Member

mydea commented Jan 12, 2023

@lforst your concern turned out to be valid: With this plugin, the generated source maps seem to get majorly destroyed. However, I can simply disable changing source maps within the plugin. Not sure if I'm missing something crucially here but I think this is okay, as the only thing that changes in our transpiled files is the path inside import/require statements (i.e. s/node_modules/ext/). Not 100% sure about this, though

If we could just disable sourcemaps here, I think that would be fine!

@lforst
Copy link
Member

lforst commented Jan 12, 2023

@lforst your concern turned out to be valid: With this plugin, the generated source maps seem to get majorly destroyed: The maps dont contain any sources or sourcesContent information anymore, which they did before. However, I can simply disable changing source maps within the plugin. Not sure if I'm missing something crucially here but I think this is okay, as the only thing that changes in our transpiled files is the path inside import/require statements (i.e. s/node_modules/ext/). Not 100% sure about this, though

I don't care too much but this will break downstream source maps of our SDK/Replay.

@Lms24
Copy link
Member Author

Lms24 commented Jan 13, 2023

Closing, as it messes up source maps :(

@Lms24 Lms24 closed this Jan 13, 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.

Replay module: Cannot find module './node_modules/rrweb/es/rrweb/packages/rrweb/src/entries/all.js'
3 participants