Azure-sdk-for-js: [Service Bus] Long term fix for duplication of module references by rush + node-resolve rollup plugin

[ramya0820]

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.

• [x] Investigate what is causing duplication of the Buffer module on disabling the dedupe flag on node-resolve plugin
• [x] Identify suitable fix and deploy

[ramya0820]

Following are some notes that could help with further investigation from Eng Sys side -

Qs to answer:

• Why is duplication happening?
• Why is duplication bad?
• How does de-dupe in rollup work?
• How is the build step making a difference to rollup output?

Things that can be tried:

• If allowing dupe vs de-dupe creates any different outputs with using just npm
• Check role of amqp-common by not having rush refer to local instance and pull it from registry instead.

[KarishmaGhiya]

https://www.npmjs.com/package/rollup-plugin-node-resolve/v/4.2.0 See the Usage section for:

// Force resolving for these modules to root's node_modules that helps
      // to prevent bundling the same package multiple times if package is
      // imported from dependencies.
      dedupe: [ 'react', 'react-dom' ], // Default: []

Changelog for this:
https://diff.intrinsic.com/rollup-plugin-node-resolve/4.1.0/4.2.0

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
Also we are using that exact version 4.2.0 of rollup-plugin-node-resolve https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/servicebus/service-bus/package.json#L95
@ramya0820 @mikeharder

[kurtzeborn]

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.

[ramya0820]

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).

[ramya0820]

Looks like something changed with rollup even with Service Bus since I last worked on it.
As I'm working on testing few things related to #3471 / #2809, noticed that even for Service Bus, needing the same additional de-dupe entries to make it work with npm. I don't think the short term workaround for Service Bus works either. We'll need to likely skip running browser tests on CI altogether for now, unless it's with npm I think or some other bundler as discussed earlier today.

cc @ramya-rao-a @bterlson @kurtzeborn @mikeharder @bterlson @KarishmaGhiya

[KarishmaGhiya]

I have created a minimal repro here : https://github.com/KarishmaGhiya/module-duplication-rollup-rush

[KarishmaGhiya]

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 : https://github.com/KarishmaGhiya/module-rollup-buffer/pull/3/commits/e04a2dd75c8db06f36a56f0f368f6744d7c70cb1
This is the PR -
https://github.com/KarishmaGhiya/module-rollup-buffer/pull/3/commits

cc: @mikeharder @bterlson

[ramya0820]

Some more notes and things to do next are:

• The code we have so far at https://github.com/KarishmaGhiya/module-rollup-buffer/pull/3/commits works with npm. And duplication of Buffer in rolled up output was noted (though we only used npm). However, the tests still pass inspite of duplicated entries in rolled up file.
• Replicate workspace and run only rush commands and see if behavior is same.
• Modify the way we are using Buffer, instead of the built in Buffer module, we can try using the import { Buffer } from "buffer";
  may also need allowSyntheticDefaultImports in your tsconfig

If the tests do not fail with Rush, then we haven't been able to exactly reproduce the problem.
And we may need to look into usage of Buffer again, and take a closer look at implementation of Rhea.
I think Rhea extends and exposes its own version/wrapper of Buffer as 'Buffer', but need to double check on this.

[ramya0820]

@KarishmaGhiya
Looks like https://github.com/KarishmaGhiya/module-rollup-buffer/pull/3/commits is not configured to run with rush as the standard rush commands - reset-workspace update build are failing with multiple instances of

 ERROR  No package.json (or package.yaml, or package.json5) was found

[KarishmaGhiya]

@ramya0820 The rush reset-workspace is not a standard rush command, it has a script that is configured with our main azure-sdk-for-js repo. I have just the minimal rush commands configured. The build should work on the master. If the build is failing with the PR then it is because of the lib-bee reference in the lib-aaa 's package.json. We need to update it like => lib-bee: 1.0.0

[ramya0820]

Update:
Tests are not failing with rush, had created a separate workspace and cloned the code from PR with above change.
Thus, the issue has not been reproduced taking the current setup of
lib-aaa -> Buffer
lib-bee -> Buffer

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.
Or, use Buffer and try to mimic the way it's being used by Service Bus, Rhea, Rhea-promise and amqp-common.

Agree? Any thoughts on which direction we should go next?
@bterlson @mikeharder @ramya-rao-a

[mikeharder]

While we don't fully understand why the dedupe is needed, we're pretty sure it's related to how pnpm installs dependencies in a tree rather than flattened, which I believe is exactly what the dedupe option was added for:

https://github.com/rollup/rollup-plugin-node-resolve/pull/201

As long the generated browser bundle looks correct and passes tests, I think it's fine to use the dedupe option.

[bterlson]

I did some more investigation today; found that there is a bug in the resolve package used by rollup-plugin-node-resolve where it will preserve symlinks in certain situations, even when preserveSymlinks is set to false. A fix is being worked on as we speak, and it may address all of our duplicate buffer issues without needing the dedupe hack (it will certainly address one duplicate, and hopefully the second 🤞).

[mikeharder]

Re-opening for tracking until https://github.com/browserify/resolve/issues/196 is resolved

[bterlson]

Confirming that with the fix for browserify/resolve#196 and the dedupe config removed, there are no duplicate copies of buffer in the bundle!