Skip to content

fix(commonjs): resolve export "exports" not found#1363

Merged
lukastaegert merged 1 commit intomasterfrom
commonjs/exports-not-found
Dec 15, 2022
Merged

fix(commonjs): resolve export "exports" not found#1363
lukastaegert merged 1 commit intomasterfrom
commonjs/exports-not-found

Conversation

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Dec 2, 2022

Rollup Plugin Name: 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

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Previously, this plugin relied on "synthetic named exports" to get a reference for the module.exports export of a module in a way that would update if module.exports is reassigned.
This apparently caused issues, so this PR changes that to instead use a getter and setter that updates exports if module.exports is reassigned.

shellscape
shellscape approved these changes Dec 5, 2022
View reviewed changes
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.

Feel free to merge 🎉

@lukastaegert
Copy link
Member Author

lukastaegert commented Dec 5, 2022

Currently thinking about this, maybe I know a solution that is even more CommonJS spec compliant, let me check. Will convert it to draft for a moment.

@lukastaegert lukastaegert marked this pull request as draft December 5, 2022 16:02
@lukastaegert lukastaegert marked this pull request as ready for review December 15, 2022 04:58
@lukastaegert
Copy link
Member Author

lukastaegert commented Dec 15, 2022

Turns out my other idea did not really work out. As I already got the go for this, I will merge this one now.

@lukastaegert lukastaegert merged commit 39a39d7 into master Dec 15, 2022
@lukastaegert lukastaegert deleted the commonjs/exports-not-found branch December 15, 2022 04:59
@wizardist
Copy link

wizardist commented Dec 21, 2022

@lukastaegert would you consider merging a patch to replace get/set with Object.defineProperty()? It would help building for some ES3 runtimes, incl. older Rhino builds. Or is it an absolute no go?

@lukastaegert
Copy link
Member Author

lukastaegert commented Dec 21, 2022

The code in question is theoretically included for every single CommonJS module. Using Object.defineProperty can blow the code up quite a bit and potentially harm tree-shaking for everyone. Can't you run a code transform plugin to rewrite the accessors?

@wizardist
Copy link

wizardist commented Dec 21, 2022

Yes, that’s our coping strategy for the time being. Thanks!

@akrawitz akrawitz mentioned this pull request Jan 16, 2023
Open
@sapphi-red sapphi-red mentioned this pull request Feb 14, 2023
Closed
7 tasks
@paescuj paescuj mentioned this pull request Feb 16, 2023
Merged
9 tasks
@d-fischer d-fischer mentioned this pull request Feb 26, 2023
Closed
@Jason3S Jason3S mentioned this pull request Feb 27, 2023
Closed
@lukastaegert lukastaegert mentioned this pull request Feb 27, 2023
Closed
9 tasks
coderaiser added a commit to coderaiser/plugins that referenced this pull request Feb 28, 2023
@coderaiser
Revert "fix(commonjs): resolve export exports not found (rollup#1363)"
3f8de2a 
This reverts commit 39a39d7.
@charliecreates charliecreates bot mentioned this pull request Feb 16, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shellscape shellscape shellscape approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[@rollup/plugin-commonjs] does not provide an export named 'exports'

3 participants

@lukastaegert @wizardist @shellscape