[OpenAI] Bundle using rollup v3 #27258
Conversation
deyaaeldeen
requested review from
ckairen,
jeremymeng and
minhanh-phan
as code owners
deyaaeldeen
force-pushed
the
openai/rollup3
branch
from
9050842
to
cc7beb3
Compare
deyaaeldeen
force-pushed
the
openai/rollup3
branch
from
cc7beb3
to
fd484bc
Compare
|
API change check API changes are not detected in this pull request. |
There was a problem hiding this comment.
Nice! Was there anything tricky about the rollup upgrade?
/cc @jeremymeng since we probably want to upgrade across the repro soon
|
This is nice! I think a few pure ESM dependencies are causing problem to tests so we haven't upgrade them. After we bump rollup to v3, and add next ESM depedency, we could probably refactoring the config to be shareable. Deferred import pattern looks a bit unusal comparing to the synchronous one though. |
@xirzec The main one is that I couldn't use the |
Yeah, I was considering if we wanted to make some utility helper for this kind of thing, though I'm not sure if rollup would be able to analyze it if we abstracted it (e.g. by passing a string for the module name) |
It is pretty much a requirement for ESM-only dependencies if they should be imported in cjs. ESM loads modules asynchronously so cjs will need to load them asynchronously as well. There has been discussion in adding a sync dynamic import function in NodeJS but it didn't go anywhere, see nodejs/modules#454 |
### Packages impacted by this PR All libraries ### Issues associated with this PR Fixes #26665 ### Describe the problem that is addressed by this PR We need to upgrade Rollup to v3 so our cjs bundles could import ES modules among other goodness. ### 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? See the linked issue for discussion of the upgrade. Credit to @xirzec for writing up the sourcemaps plugin in-house replacement! ### Are there test cases added in this PR? _(If not, why?)_ N/A ### Provide a list of related PRs _(if any)_ [N/A](#27258) ### 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)
Packages impacted by this PR
@azure/openai
Issues associated with this PR
Fixes #27227
Fixes #27226
Describe the problem that is addressed by this PR
@azure/openaidepends on v4 offormdata-nodeand v1.7.2 ofform-data-encoder. However, those are outdated versions and we should depend on recent versions of those libraries instead but the recent versions are ESM-only. I couldn't find a way to bundle ESM code into cjs using rollup v2, our current rollup version. With rollup v3, you can bundle ESM-only code if you import it with dynamic imports and this PR does exactly that.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)
N/A
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists