-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Service Bus] Long term fix for duplication of module references by rush + node-resolve rollup plugin #3326
Comments
Following are some notes that could help with further investigation from Eng Sys side - Qs to answer:
Things that can be tried:
|
https://www.npmjs.com/package/rollup-plugin-node-resolve/v/4.2.0 See the Usage section for:
Changelog for this: https://github.com/rollup/rollup-plugin-node-resolve/releases/tag/v4.2.0 Going through the above links, this seems like the duplication of module references was expected by their code change when they moved from version 4.1.0 to 4.2.0 for rollup-plugin-node-resolve. They did expect users to use dedupe which is why they even added in that option |
Next step: let's create a minimal repro of this (a depends on b, and both a and b depend on c. c is a third party dependency) and then experiment with fixes in an isolated situation. Aside: While we currently use rollup, one possible investigation path would be to switch to webpack, parcel or pika. This represents a bunch of work to switch though, so this would need to wait until after the previews are released. |
Also, please refer to thread on #2809 for more context on what was attempted for EventHubs' browser tests to work with Rush. For rollup to work with npm 4 more de-dupe entries were required. This is opposite of what was needed for Service Bus, where npm didn't require it and rush did. For Event Hubs, we haven't been able to get a short-term fix either (yet). |
Looks like something changed with rollup even with Service Bus since I last worked on it. cc @ramya-rao-a @bterlson @kurtzeborn @mikeharder @bterlson @KarishmaGhiya |
I have created a minimal repro here : https://github.com/KarishmaGhiya/module-duplication-rollup-rush |
Update: @ramya0820 and I were able to repro the duplication of buffer issue. Here's the latest commit of the PR to the repro here : KarishmaGhiya/module-rollup-buffer@e04a2dd cc: @mikeharder @bterlson |
Some more notes and things to do next are:
If the tests do not fail with Rush, then we haven't been able to exactly reproduce the problem. |
@KarishmaGhiya
|
@ramya0820 The |
Update: We might have to likely try and mimic the concept of exposing global variable in the minimal package ‘C’ repro, as opposed to using ‘Buffer’ package which is a bit more complicated. Agree? Any thoughts on which direction we should go next? |
While we don't fully understand why the rollup/rollup-plugin-node-resolve#201 As long the generated browser bundle looks correct and passes tests, I think it's fine to use the |
I did some more investigation today; found that there is a bug in the |
Re-opening for tracking until browserify/resolve#196 is resolved |
Confirming that with the fix for browserify/resolve#196 and the dedupe config removed, there are no duplicate copies of buffer in the bundle! |
- Update transitive dependency resolve to 1.12.0 - Fixes issue with symlink resolution which required dedupe workaround - browserify/resolve#196 - Fixes Azure#3326
- Update transitive dependency resolve to 1.12.0 - Fixes issue with symlink resolution which required dedupe workaround - browserify/resolve#196 - Fixes Azure#3326
- Update transitive dependency resolve to 1.12.0 - Fixes issue with symlink resolution which required dedupe workaround - browserify/resolve#196 - Move buffer to full dependency of service-bus - Packages required for browser bundles should be full dependencies - Improves customer experience when generating bundles from our packages - Add dependencies buffer and process to event-hubs - Required to generate browser bundle - Fixes Azure#3326
- Update transitive dependency resolve to 1.12.0 - Fixes issue with symlink resolution which required dedupe workaround - browserify/resolve#196 - Move buffer to full dependency of service-bus - Packages required for browser bundles should be full dependencies - Improves customer experience when generating bundles from our packages - Add dependencies buffer and process to event-hubs - Required to generate browser bundle - Fixes Azure#3326
- Update transitive dependency resolve to 1.12.0 - Fixes issue with symlink resolution which required dedupe workaround - browserify/resolve#196 - Move buffer to full dependency of service-bus - Packages required for browser bundles should be full dependencies - Improves customer experience when generating bundles from our packages - Add dependencies buffer and process to event-hubs - Required to generate browser bundle - Fixes Azure#3326
- Update transitive dependency resolve to 1.12.0 - Fixes issue with symlink resolution which required dedupe workaround - browserify/resolve#196 - Depends on rollup-plugin-commonjs@10.0.2 - Supports preserveSymlinks:false - rollup/rollup-plugin-commonjs#400 - Move buffer to full dependency of service-bus - Packages required for browser bundles should be full dependencies - Improves customer experience when generating bundles from our packages - Add dependencies buffer and process to event-hubs - Required to generate browser bundle - Fixes #3326
The fix deployed for #3119 involves removing duplicate references to Buffer module after they have been generated.
This issue is to investigate why the duplication is happening, and deploy a long term fix for same issue that would not require us to post-process and remove the duplicates.
The text was updated successfully, but these errors were encountered: