Azure-sdk-for-js: [Storage] Fix rollup warnings

[HarshaNalluru]

[HarshaNalluru]

Fixes some warnings - #4074

[HarshaNalluru]

Warnings

[ramya-rao-a]

Is this still applicable now that we are removing the circular dependencies by moving the code to the same file?

[HarshaNalluru]

Regarding eval warning -
It's unrelated to storage and should be tracked under the common recorder.

Nevertheless, logged an issue in the sinonjs/nise repo regarding eval.
https://github.com/sinonjs/nise/issues/110

[HarshaNalluru]

Current warnings -

Circular references are being fixed at #5993 and #5998

[ramya-rao-a]

@willmtemple Now that the PRs for fixing the circular references are merged, can you update this issue with a screenshot of remaining warnings from rollup?

[ramya-rao-a]

Moving this issue to the Jan milestone to follow up on leftover warnings if any

[jeremymeng]

Current warnings (test only)

[jeremymeng]

Adding fs to output.globals gets rid of the last warning, but not a proper fix.

[jeremymeng]

Some ideas from discussion with @HarshaNalluru

• split the nise recorder into a recorder.browser.ts or similar so fs/fs-extra used in nock recorder won't get bundled for browser
• ~basekarma.conf.ts provides a helper that is used by karam config of all libraries, but it is only used when running karma so ideally shouldn't be bundled into test rollup.~ edited: no test code is importing this so it is not bundled.

[jeremymeng]

After #6909 now storage rollup warnings are of two types, fs related, and nise related. Both are common to all libraries. So removing Storage labels and use this to track fixing rollup warnings for all libraries if we want to (note that there are only warnings from test rollup. production rollup is all green).

[jeremymeng]

[HarshaNalluru]

Thanks!!!

[ramya-rao-a]

I don't see any more rollup warnings when running rushx build for

• storage-blob
• storage-queue
• storage-file-share
• storage-file-datalake

@jeremymeng, @HarshaNalluru Can we close this?

[jeremymeng]

The warnings are from build:test script

[willmtemple]

The remaining warnings that we are seeing here in test are some isolated warnings about dependencies not matching that will be solved by rollup config abstraction work and these warnings about the use of eval in the Nise package.

Marking this up for grabs if anyone wants to take a stab at the Nise issue, since it needs a little investigation. We can probably handle this warning by silencing it in the test configurations.

[HarshaNalluru]

I had an issue logged in the nise repo https://github.com/sinonjs/nise/issues/110 long back, which has been moved to https://github.com/sinonjs/fake-timers/issues/319 and seems like it is actively being worked on.

We can disable the warnings through the rollup config.

Regarding "fs-extra", I have a PR to remove it completely from the repo https://github.com/Azure/azure-sdk-for-js/pull/9402
Somehow, the keyvault-certificates CI failed and blocked the PR. I'll revive the PR back and take a stab at it again!

[willmtemple]

@HarshaNalluru I did end up including an inhibitor for the Nise/Sinon eval warning in the proposed shared rollup configuration. #10923

[ramya-rao-a]

@willmtemple, Is there any other pending work here to be done other than using the shared rollup config once it is available?

[willmtemple]

@ramya-rao-a In theory, it is the same as using the shared rollup config. In practice, adapting to it may require some code changes if storage's code is not compatible with the shared rollup config out-of-the-box. It will depend a little bit once I can dig into it.