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): __moduleExports in multi entry + inter dependencies #415

Merged

Conversation

danielgindi
Copy link
Contributor

@danielgindi danielgindi commented May 25, 2020

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).

@shellscape
Copy link
Collaborator

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

@lukastaegert
Copy link
Member

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

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.

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

packages/commonjs/src/index.js Outdated Show resolved Hide resolved
var input1 = 1;

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

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
Contributor Author

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
Contributor Author

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
Member

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
Contributor

This is great, thank you @danielgindi!

LarsDenBakker pushed a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020
…ollup#415)

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

* Added form tests outputs to .prettierignore
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.

4 participants