Skip to content

fix(commonjs): __moduleExports in multi entry + inter dependencies#415

Merged
shellscape merged 2 commits intorollup:masterfrom
danielgindi:bugfix/cjs_multi_entry_exports
Jun 1, 2020
Merged

fix(commonjs): __moduleExports in multi entry + inter dependencies#415
shellscape merged 2 commits intorollup:masterfrom
danielgindi:bugfix/cjs_multi_entry_exports

Conversation

@danielgindi
Copy link
Copy Markdown
Contributor

@danielgindi danielgindi commented May 25, 2020 •
edited
Loading

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

List any relevant issue numbers: #411

Description

When a module is a main entry point but still imported by another module (due to multiple entries feature), we can now detect that and properly export __moduleExports.

Tests

I've had to modify the form tester to support multi-entry situations. (In the function tester it does not make a lot of sense as we run the code in memory where it would expect to have other generated outputs to exist on disk for importing).

@danielgindi danielgindi force-pushed the bugfix/cjs_multi_entry_exports branch from 419c379 to 001a71d Compare May 25, 2020 18:09
@shellscape
Copy link
Copy Markdown
Collaborator

shellscape commented May 29, 2020

@danielgindi please have a peek at that snapshot when you have the chance and then we'll merge

@lukastaegert
Copy link
Copy Markdown
Member

lukastaegert commented May 30, 2020

I'm also gonna have a small peek in a second

lukastaegert
lukastaegert reviewed May 30, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Two comments, and I am also missing the updated snapshots in the markdown file, did you commit this?

packages/commonjs/test/fixtures/form/multi-entry-module-exports/output1.js
var input1 = 1;

export default input1;
export { input1 as __moduleExports };
Copy link
Copy Markdown
Member

@lukastaegert lukastaegert May 30, 2020

Choose a reason for hiding this comment

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

So you said we cannot get rid of __moduleExports because it will trigger interop handlers when using the default export? The thing is, this will now taint entry points in a way that users will notice there is suddenly a new named export that does not belong here. So one could wonder if for entry points, we might rather use the default even though it triggers interop? But I am undecided, what do you think?

Copy link
Copy Markdown
Contributor Author

@danielgindi danielgindi May 30, 2020

Choose a reason for hiding this comment

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

Well this only happens for entry points that are being imported by other code paths in the bundle.
So in most use cases - it will not be exported.
And in the use cases where it does - it's due to its use by one of the other entry points.

Unfortunately I don't see any way around it without breaking code... But if I think of some other trick to get rid of this, I'll make a PR :-)

@danielgindi danielgindi force-pushed the bugfix/cjs_multi_entry_exports branch 3 times, most recently from 4ca9019 to 6a7ba9d Compare May 30, 2020 18:10
@danielgindi danielgindi force-pushed the bugfix/cjs_multi_entry_exports branch from 6a7ba9d to 322de1d Compare May 30, 2020 18:13
@danielgindi
Copy link
Copy Markdown
Contributor Author

danielgindi commented May 30, 2020

I'm having issues with prettier modifying form test outputs, where the transformer creates a var and prettier converts to a const. This forces me to bypass commit hooks.
I'm thinking we need to change the var outputted from the transformer to const, and configure prettier to avoid form outputs.

@lukastaegert
Copy link
Copy Markdown
Member

lukastaegert commented May 30, 2020

I'm not sure about the var, would be a first we are creating ES2015+ output, except for the imports and exports of course. Seems like a waste to just use a const but not go all-in with the other features like arrow functions. For now, I would rather keep it a var.

But yes, test output should not be covered by prettier. Did you try extending the .prettierignore file? Not sure in how far it supports globs, I hope so. Otherwise one could try to tweak the lint-staged config in the central package.json file, but that seems harder to me.

@shellscape shellscape merged commit 7ae5390 into rollup:master Jun 1, 2020
@FredKSchott
Copy link
Copy Markdown
Contributor

FredKSchott commented Jun 3, 2020

This is great, thank you @danielgindi!

LarsDenBakker pushed a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020
@danielgindi @LarsDenBakker
fix(commonjs): __moduleExports in multi entry + inter dependencies (r…
b07edae 
…ollup#415)

* fix(commonjs): __moduleExports in multi entry + inter dependencies

* Added form tests outputs to .prettierignore
@dependabot dependabot bot mentioned this pull request Mar 18, 2021
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

@lukastaegert lukastaegert lukastaegert approved these changes

@NotWoods NotWoods Awaiting requested review from NotWoods

@Andarist Andarist Awaiting requested review from Andarist

@guybedford guybedford Awaiting requested review from guybedford

@keithamus keithamus Awaiting requested review from keithamus

@TrySound TrySound Awaiting requested review from TrySound

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@danielgindi @shellscape @lukastaegert @FredKSchott