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

[core-paging] Include esm version too in addition to CommonJS #11563

Merged
merged 3 commits into from
Sep 30, 2020
Merged

[core-paging] Include esm version too in addition to CommonJS #11563

merged 3 commits into from
Sep 30, 2020

Conversation

jeremymeng
Copy link
Member

No description provided.

@ghost ghost added the Azure.Core label Sep 30, 2020
@jeremymeng jeremymeng mentioned this pull request Sep 30, 2020
Closed
6 tasks
xirzec
xirzec approved these changes Sep 30, 2020
View reviewed changes
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

I am surprised that we were blithely shipping our CommonJS version inside a dist-esm folder previously.

@ramya-rao-a
Copy link
Contributor

@xirzec, @jeremymeng, What should be the guideline here?
Is this something we have been doing in other packages but missed in core-paging?
Is this something the linter could have caught?

cc @deyaaeldeen, @willmtemple

@deyaaeldeen
Copy link
Member

deyaaeldeen commented Sep 30, 2020 •
edited
Loading

@ramya-rao-a I turned off the related linter rules here #11274. With this change, @jeremymeng can turn those back on:

  • ts-package-json-main-is-cjs
  • ts-package-json-module
  • ts-package-json-files-required

Please verify that rushx lint does not produce any errors.

@jeremymeng
Copy link
Member Author

Is this something we have been doing in other packages but missed in core-paging?

yes, other libraries are using rollup. core-paging is special as it only has one file which only imports @core-asynciterator-polyfill and we probably didn't pay enough attention.

@jeremymeng
Copy link
Member Author

Please verify that rushx lint does not produce any errors.

I turned them on and eslint is happy.

deyaaeldeen
deyaaeldeen approved these changes Sep 30, 2020
View reviewed changes
Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@jeremymeng jeremymeng merged commit 296bc1c into Azure:master Sep 30, 2020
@jeremymeng jeremymeng deleted the core-paging-esm branch September 30, 2020 17:47
@jeremymeng jeremymeng mentioned this pull request Jun 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deyaaeldeen deyaaeldeen deyaaeldeen approved these changes

@xirzec xirzec xirzec approved these changes

@bterlson bterlson Awaiting requested review from bterlson

@daviwil daviwil Awaiting requested review from daviwil

Assignees
No one assigned
Labels
Azure.Core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jeremymeng @ramya-rao-a @deyaaeldeen @xirzec