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.

Sign up for GitHub

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

Jump to bottom

Upgrade dev dependency rollup to v3 #26665

Closed
jeremymeng opened this issue Jul 28, 2023 · 5 comments · Fixed by #27285
Closed

Upgrade dev dependency rollup to v3 #26665

jeremymeng opened this issue Jul 28, 2023 · 5 comments · Fixed by #27285
Assignees
@jeremymeng
Labels
Client This issue points to a problem in the data-plane of the library. EngSys This issue is impacting the engineering system.

Comments

@jeremymeng
Copy link
Member

Here's a v3 migration guide: https://rollupjs.org/migration/#changed-defaults

It looks that we are not impact by its breaking changes.
@jeremymeng jeremymeng added EngSys This issue is impacting the engineering system. Client This issue points to a problem in the data-plane of the library. labels Jul 28, 2023
@jeremymeng jeremymeng self-assigned this Jul 28, 2023
@jeremymeng
Copy link
Member Author

to upgrade:

rush add --dev -m -p @rollup/plugin-node-resolve@^15.1.0 -p rollup@^3.0.0 -p @rollup/plugin-commonjs@^25.0.0

though currently blocked by maxdavidson/rollup-plugin-sourcemaps#127

@jeremymeng
Copy link
Member Author

jeremymeng commented Sep 27, 2023 •
edited
Loading

@deyaaeldeen was able to use v3 here: #27258

@deyaaeldeen deyaaeldeen mentioned this issue Sep 27, 2023
Merged
3 tasks
@xirzec
Copy link
Member

xirzec commented Sep 27, 2023

https://rollupjs.org/configuration-options/#output-sourcemap maybe we don't need the plugin anymore?

@deyaaeldeen
Copy link
Member

deyaaeldeen commented Sep 28, 2023 •
edited
Loading

This plugin loads the source maps from external dependencies to be included in the bundle using source-map-resolve. However, the latter is deprecated even though it has 2M weekly downloads and there is no equivalent library available AFAICT. I am not sure if the reasoning of the deprecation is that big of a deal but let me know what do you think. I think our options basically this:

  1. create our own private copy of rollup-plugin-sourcemaps updated to work with rollup v3
  2. upgrade to rollup v3 without this plugin and pass the issue to our cjs customers. Stacktraces in our browser testing will also be impacted, they will point to the JS ESM modules instead of the TS modules, e.g. EDIT: This part can be remedied by using the typescript rollup plugin.
Error: ENDPOINT is not defined
[browser-tests]         at assertEnvironmentVariable (../../test-utils/recorder/dist-esm/src/utils/filePathGenerator.js:6:1 <- dist-test/index.browser.js:3289:19)
[browser-tests]         at createClient$1 (dist-esm/test/public/utils/recordedClient.js:47:18 <- dist-test/index.browser.js:50828:26)
[browser-tests]         at Context.<anonymous> (dist-esm/test/public/samples.spec.js:28:15 <- dist-test/index.browser.js:58862:22)
  1. stay on rollup v2 and miss support for importing esm modules in our cjs bundles among other potential features

@deyaaeldeen deyaaeldeen mentioned this issue Sep 28, 2023
Merged
3 tasks
@jeremymeng jeremymeng reopened this Sep 29, 2023
@deyaaeldeen deyaaeldeen reopened this Oct 1, 2023
@deyaaeldeen deyaaeldeen mentioned this issue Oct 2, 2023
Merged
3 tasks
deyaaeldeen added a commit that referenced this issue Oct 10, 2023
@deyaaeldeen
[Event Hubs] Use dev-tool's bundle (#27305)
075c4d1 
Successful live runs:
- [Schema
Registry](https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3142121&view=results)
- [Event
Hubs](https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3143866&view=results)

### Packages impacted by this PR
@Azure/event-hubs

### Issues associated with this PR
#26665

### Describe the problem that is addressed by this PR
Move from rollup v2 to v3.

### What are the possible designs available to address the problem? If
there are more than one possible design, why was the one in this PR
chosen?
Node polyfilling is not doing the right thing in Event Hubs for some
reason and I had to disable it. In particular, the buffer API was
causing bad generated tokens that are used for SAS authentication with
the service. My approach is to continue to rely on the inject plugin to
inject imports to the polyfill buffer dependency. However, since we
disabled node polyfilling for event hubs, we have to do manual
polyfilling for all libraries that depend on event hubs which are
basically the schema registry serializers. This PR manually polyfills
streams used by avsc and suppresses rollup warnings for missing other
node builtins since they're not needed in our scenarios.

The advantage of this work is that we no longer need custom rollup
configurations for Event Hubs or Schema Registry.

### Are there test cases added in this PR? _(If not, why?)_
N/A

### Provide a list of related PRs _(if any)_
N/A

### Command used to generate this PR:**_(Applicable only to SDK release
request PRs)_

### Checklists
- [x] Added impacted package name to the issue description
- [ ] Does this PR needs any fixes in the SDK Generator?** _(If so,
create an Issue in the
[Autorest/typescript](https://github.com/Azure/autorest.typescript)
repository and link it here)_
- [ ] Added a changelog (if necessary)
@deyaaeldeen deyaaeldeen mentioned this issue Oct 10, 2023
Merged
3 tasks
deyaaeldeen added a commit that referenced this issue Oct 10, 2023
@deyaaeldeen
[Service Bus] Upgrade rollup to v3 (#27363)
e1b30ef 
[Successful live
test](https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3150973&view=results)

### Packages impacted by this PR
@Azure/service-bus

### Issues associated with this PR
#26665

### Describe the problem that is addressed by this PR
Upgrade rollup from v2 to v3

### What are the possible designs available to address the problem? If
there are more than one possible design, why was the one in this PR
chosen?
N/A

### Are there test cases added in this PR? _(If not, why?)_
N/A

### Provide a list of related PRs _(if any)_
#27305

### Command used to generate this PR:**_(Applicable only to SDK release
request PRs)_

### Checklists
- [x] Added impacted package name to the issue description
- [ ] Does this PR needs any fixes in the SDK Generator?** _(If so,
create an Issue in the
[Autorest/typescript](https://github.com/Azure/autorest.typescript)
repository and link it here)_
- [ ] Added a changelog (if necessary)
@deyaaeldeen
Copy link
Member

This has been addressed.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jeremymeng jeremymeng

Labels
Client This issue points to a problem in the data-plane of the library. EngSys This issue is impacting the engineering system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[dev-tool] Upgrade to rollup v3 Azure/azure-sdk-for-js
3 participants
@xirzec @deyaaeldeen @jeremymeng