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.

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

fix(commonjs)!: check if defaultIsModuleExports is auto for getDefaultExportFromCjs #1358

Merged
merged 5 commits into from Dec 18, 2022

Conversation

flyingbirdhub
Copy link
Contributor

@flyingbirdhub flyingbirdhub commented Nov 29, 2022

if defaultIsModuleExports option is auto, the export may should be wrapped with getDefaultExportFromCjs

Rollup Plugin Name: @rollup/plugin-commonjs

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

some break changes, when run test command, i do not know if it is right, but here need to be fixed in some way

List any relevant issue numbers:
#1209

Description

…ion is auto, if defaultIsModuleExports option is auto, the export may should be wrapped with getDefaultExportFromCjs
@shellscape
Copy link
Collaborator

Tests are required for fixes and features.

@flyingbirdhub
Copy link
Contributor Author

Tests are required for fixes and features.

i have fixed some test cases which fail to pass the tests, and add a test case for the problem solved by this mr

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@shellscape shellscape changed the title fix: getEsImportProxy should check whether defaultIsModuleExports opt… fix(commonjs): check if defaultIsModuleExports is auto for getDefaultExportFromCjs Dec 17, 2022
Copy link
Collaborator

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

@flyingbirdhub thanks for your patience on this. we're ready to merge this but there is a conflict that needs to be addressed first. please take a look

@shellscape shellscape changed the title fix(commonjs): check if defaultIsModuleExports is auto for getDefaultExportFromCjs fix(commonjs)!: check if defaultIsModuleExports is auto for getDefaultExportFromCjs Dec 17, 2022
@flyingbirdhub
Copy link
Contributor Author

flyingbirdhub commented Dec 18, 2022

@flyingbirdhub thanks for your patience on this. we're ready to merge this but there is a conflict that needs to be addressed first. please take a look

@shellscape i have solved this conflict, pls task a look, thanks

@shellscape shellscape merged commit 4766d93 into rollup:master Dec 18, 2022
@shellscape
Copy link
Collaborator

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants