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

feat(commonjs): set syntheticNamedExports for commonjs modules #149

Merged

Conversation

LarsDenBakker
Copy link
Contributor

@LarsDenBakker LarsDenBakker commented Jan 8, 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: fixes #142

Description

Fixes #142. it's not as straightforward as simply setting syntheticNamedExports to true, because rollup throws an error when there is no default export.

I've added various test cases, it seems to work as expected.

It is a breaking change because we are removing the namedExports option, and require at least rollup v2.3.4 or higher. Instead of manually defining named exports, rollup now handles this automatically for you.

@LarsDenBakker LarsDenBakker force-pushed the feat/commonjs-synthetic-named-exports branch from 0850f3e to 6f40b47 Compare January 8, 2020 07:20
@@ -2,4 +2,3 @@ import * as x from './answer';

t.truthy('answer' in x);
t.truthy('default' in x);
t.truthy(!('__esModule' in x));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change, __esModule is available as a named import. In my opinion this is correct, since it's exported from the commonjs module. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lukastaegert this is something you'd have to approve. not speaking for the other maintainers, but I lack the knowledge to know if this is a good change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually the list of exports is detected via Object.keys which would skip the __esModule property since it is defined to be non-enumerable. I would be interested in how this list is being gathered that it is including it.

@LarsDenBakker
Copy link
Contributor Author

This is dependent on rollup/rollup#3319

@LarsDenBakker LarsDenBakker force-pushed the feat/commonjs-synthetic-named-exports branch 2 times, most recently from 755d53f to fefb4ef Compare January 8, 2020 07:32
@@ -393,20 +393,6 @@ test('does not process the entry file when it has a leading "." (issue #63)', as
await t.notThrowsAsync(executeBundle(bundle, t));
});

test('does not reexport named contents', async (t) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you comment on why this test was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It tests that the plugin throws on unknown named exports. This is exactly what the syntheticNamedExports enables.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. (forgive my ignorance here, the commonjs plugin isn't all that familiar yet.) would we want to allow people to disable syntheticNamedExports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem! That's indeed something to consider. Personally I'd say no, commonjs is a dynamic module format by design and this aligns the behavior of rollup with other bundlers like webpack. I can't think of a valid use case for not wanting this behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. We'd probably do a major version on this, and if anyone complained maybe take a second look at the ability to turn it off.

@lukastaegert
Copy link
Member

We definitely need to wait for rollup/rollup#3319 to do a new round of testing. However one thing I was hoping to see in this PR was nuking the namedExports option. There should be no advantage in having it still around, and it is IMO the single biggest nuisance of this plugin that is made obsolete by this PR.

Since this is a breaking release anyway, and likely will require some further iteration after its initial release, I would suggest to make the option basically a no-op that prints a warning that it is no-longer needed.

BTW since you only have access to Rollup's internal warning mechanism in plugin hooks, my recommended way to report configuration warnings would be in a buildStart hook via this.warn.

@LarsDenBakker
Copy link
Contributor Author

I removed the namedExports option

pnpm-lock.yaml Outdated
@@ -20,8 +20,13 @@ importers:
pnpm: 4.6.0
prettier: 1.19.1
prettier-plugin-package: [email protected]
rollup: 1.27.14
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

Merge conflicts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes something went wrong syncing, fixed it now

@btd
Copy link
Contributor

btd commented Jan 10, 2020

Probably need to restrict required rollup version in package.json somehow

@LarsDenBakker LarsDenBakker force-pushed the feat/commonjs-synthetic-named-exports branch 2 times, most recently from bb6eba9 to 9f2db99 Compare January 10, 2020 10:41
@nathancahill
Copy link

I'm using this fork in a fairly large project and it works great. Was able to drop namedExports completely.

@LarsDenBakker
Copy link
Contributor Author

I dont recommend using it until the linked PR is merged. Otherwise you will run into problems somewhere down the line.

@shellscape
Copy link
Collaborator

@LarsDenBakker looks like rollup/rollup#3295 was merged and released. Is this PR good to go?

@TrySound
Copy link
Member

TrySound commented Feb 3, 2020

@shellscape There is another one rollup/rollup#3319

@lukastaegert
Copy link
Member

Exactly. I got some feedback from @manucorporat a few days ago that he was back working on this but that was that. If nothing happens, I might also be able to take over the necessary changes myself in a few days. Otherwise this is in no fit state for production use.

@LarsDenBakker
Copy link
Contributor Author

@lukastaegert I removed the unneeded fixtures

@lukastaegert
Copy link
Member

@guybedford I know the idea and hope is to "educate" users not to use named exports with CJS but in a world where React is published as CJS while the docs have been teaching users to use named ES imports for years, it just ends up as "oh, Rollup does not work out of the box while Webpack and Parcel do, what shitty software". And also this generates bug tickets over and over.

So yes, this is like giving in, but really, I do not see CJS as a format where I care 100% about compatibility. Rollup is an ESM bundler, the goal for CJS support is just to cause as little pain to as many users as possible, at least that is my personal stance.

Continuing, I do not see a lot of benefit in making this configurable. A configuration would just be a means of restricting yourself, but why? The version that supports automatic named exports also supports always using the default and generates the same output.

As for the questions

Does a namespace import trigger all the getters of the exported module?

No they don't. The assumption is that a module with synthetic named exports has all possible exports at the same time, so it is just rendered as a property access. Actually it is not unlike external namespaces in many regards.

That also means that if you export namespaces from two synthetic named exports modules, the first will "take all", with the exception that actual named exports will always take precedence, at least that was my intention.

In a cycle between two modules with synthetic exports both importing the namespace of the other, is one namespace object always in a TDZ if accessed?

Probably. But this is true for regular Rollup code as well, at least of you reify your namespaces. If you do not, this actually quite improves circular dependency handling as everything is done via property accesses.

But instead of listing potential issues without verifying, I would recommend you rather play around with the feature yourself to see how various things are solved. It is not perfect, but a "best effort" approach definitely. You could have a look at the test cases, e.g. https://github.com/rollup/rollup/tree/master/test/function/samples/synthetic-named-exports-namespace-overrides or https://github.com/rollup/rollup/tree/master/test/chunking-form/samples/mixed-synthetic-named-exports, and there are many more

@shellscape
Copy link
Collaborator

(I think we'll need someone to post in big bold letters when this is ready for merge)

@guybedford
Copy link
Contributor

@LarsDenBakker I have no concerns over this work - my major concern is the removal of the ability to do strict interop. It is a false dichotomy to think we can only go one way here. There are multiple ships running multiple timelines here. And there's some tension between them. So all I'm asking for is that we retain a namedExports: 'strict' option or similar as an opt-in while making the default behaviour the one you are defining.

I'm very grateful for the work you have done here and already use synthetic exports - it is definitely a given and assumption that the out of the box experience should "just work" and the only way to do that is via synthetic exports. But removing the previous work breaks both backwards compatibility for existing users, while also blocking off the strict enforcement which is what I disagree with.

The good news is we don't need to have a long debate about the future here - the compromise is quite straightforward based on accepting we have different needs for different situations. But if you are interested in the larger discussion - in terms of the overall situation, there is also a longer term interop in play here. The Node.js interop will define quite strictly how ES modules that import CommonJS work, and packages that want to work on npm in Node.js will fine over the coming years that they need to obey these semantics to ensure things work for their users. In addition, jspm provides a conversion of CommonJS modules into ES modules that is done before linking packages together. As a result, the importer-specific information used by synthetic approaches is not available in this unlinked problem domain, and hence strict interop where CommonJS modules are only reflected as a { default } module again is preferable for its universality (synthetic interop causes module exports to be effectively consumer-defined). Thus over the coming years these forms of interop will come to dominate since the npm semantics of published packages are stickier semantics than those of your local application which is not part of a universal registry.

@lukastaegert

A configuration would just be a means of restricting yourself, but why?

Firstly, removing the existing configuration is effectively a breaking change. And breaking changes should always be avoided when possible. Secondly, these are linking time semantics, so should be linking time errors. The build tool runs at link time, so should throw linking errors. I don't understand why you would want to argue for less flexibility on this important problem. Is engineering the bottleneck here?

Let's move the discussion of the synthetic edge case semantics elsewhere then if it is a core thing. As mentioned I don't think it affects much right now. One interesting thought here is that namespace objects in RollupJS in future might benefit from being defined to be an object of getters to the underlying bindings to retain the live references.

@manucorporat
Copy link

As a result, the importer-specific information used by synthetic approaches is not available in this unlinked problem domain, and hence strict interop where CommonJS modules are only reflected as a { default } module again is preferable for its universality (synthetic interop causes module exports to be effectively consumer-defined).

Not sure i understand correctly your concern here, but under the hood, the synthetic named exports don't create new exports, but references the default export, keeping live references:

import {stuff} from 'react';
console.log(named);

becomes

import react from 'react';
console.log(react.stuff);

@lukastaegert
Copy link
Member

@guybedford The thing is that there never was a strict mode, it would be a completely separate engineering effort to add it, and I do not see there is user demand for this effort, at least I am not aware of issues from actual users that they want this. So if you want it, please add it, this contribution would definitely be welcome. But it would be sad to stall this PR because this feature is missing. This plugin has always auto-detected named exports, and now those auto-detected exports are accompanied by synthetic exports for the cases where auto-detection fails. If we want strict mode, then IMO that is a completely separate issue and should not affect this at all. Removing the named exports option is kind of breaking but only nominally: All setups that were working before should still be working.

@manucorporat
Copy link

manucorporat commented Apr 13, 2020

@lukastaegert if we really wanted, i guess the breaking change could be prevented (from a types perspective), but leaving the types for namedExport with a @deprecated jsdocs tag, but it doesn't do anything, since as you well pointed out synthetic named exports will do the same under the hood.

UPDATE:
However... the breaking change is unavoidable because of the peerDependency update, so never mind

@lukastaegert
Copy link
Member

but it doesn't do anything

Well, that is not exactly true, exports created via namedExports are immutable snapshots as far as I know while synthetic exports are live-bindings. Performance might be slightly better if you use namedExports (but probably negligible) while synthetic exports should work in more situations.

However... the breaking change is unavoidable because of the peerDependency update

true. As I said, I think it will be a mostly painless breaking change where the migration will be to remove the namedExports option. The question is if this really works as we envisioned it. Would be nice to get some feedback from people who are willing to try this out.

I actually found a nice way to install the plugin from this PR via https://gitpkg.now.sh/

npm install 'https://gitpkg.now.sh/LarsDenBakker/plugins/packages/commonjs?feat/commonjs-synthetic-named-exports'

@LarsDenBakker
Copy link
Contributor Author

LarsDenBakker commented Apr 13, 2020

@guybedford I thought you meant keeping the current namedExports option, where you manually specify the named exports. It sounds like there is a misunderstanding of the current behavior of the commonjs plugin.

Having a fully strict mode I think makes a lot of sense for this plugin, but I'd expect it to only handle this format:

import { createRequire } from 'module';
const require = createRequire(import.meta.url);
const siblingModule = require('./sibling-module');

Importing commonjs using standard import statement isn't valid, it doesn't work in the browser nor in node so it would always be custom behavior.

@lukastaegert
Copy link
Member

Well, it does in fact work in Node 13+ now, but it will only provide you with a single default export, which corresponds to module.exports. I think this is what Guy is aiming at. Still I want to reiterate that even currently, the namedExports option is not needed if exports are only defined and accessed in well-defined ways, in which case this plugin will automatically provide optimised output that even supports tree-shaking for CJS.
Until we have a way to get tree-shaking for object properties, for which I had plans for ages but other things were and are still more important, a strict mode cannot be recommended for web developers as they will also get bigger bundles.

@guybedford
Copy link
Contributor

Yes I did overlook the default behaviour is already similar to synthetic imports such that this doesn't actually remove or disable any strict mode.

In that case I would like to still suggest that we work towards a strict mode though in the coming months based on the reasons above.

Tools like Rollup have to first bow to the demands of users as the primary priority. But there is a greater underlying priority which is to in turn mould the ecosystem well in the process.

@LarsDenBakker
Copy link
Contributor Author

@shellscape Then I think we can merge this PR. Strict NodeJS compliant commonjs/esm interop can be implemented separately.

@nstepien

This comment has been minimized.

@shellscape
Copy link
Collaborator

@lukastaegert @guybedford still waiting for everyone to agree this is good to go. @LarsDenBakker has weighed in, in the affirmative.

@lukastaegert
Copy link
Member

I would definitely love to release this as is!

@shellscape shellscape merged commit 5d2dcf4 into rollup:master Apr 22, 2020
@shellscape
Copy link
Collaborator

2 out of 3 works for me. Merged.

@manucorporat
Copy link

Out of curiosity, when can we expect a new release of the commonjs plugin?

@shellscape
Copy link
Collaborator

Within a week I figure. There's a few more commonjs PRs open that I'd like to see part of the next publish.

@danielgindi
Copy link
Contributor

This change broke the snapshot tests. They come out differently on different node versions (10, 12, 13), and this inconsistency always breaks the snapshots test in one of the versions.

@FredKSchott
Copy link
Contributor

Congrats on merging everyone! Looking forward to testing this in Snowpack

LarsDenBakker added a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020
…p#149)

BREAKING CHANGE:

The namedExports option has been removed, now requires rollup >= 2.3.4. Instead of manually defining named exports, rollup now handles this automatically for you.

* feat(commonjs): set syntheticNamedExports for commonjs modules

* feat(commonjs): remove namedExports option

* chore: merge with upstream

* chore(commonjs): restrict rollup version

* chore(commonjs): remove unused fixtures
PeachScript added a commit to umijs/father that referenced this pull request Nov 3, 2020
PeachScript added a commit to umijs/father that referenced this pull request Nov 3, 2020
ycjcl868 added a commit to umijs/father that referenced this pull request Nov 24, 2020
* deps: upgrade to rollup 2 and upgrade related dependencies

* test: update expected output for all rollup cases

* refactor: remove useless namedExports option

ref: rollup/plugins#149

* chore: upgrade rollup to 2.33.2

Co-authored-by: 信鑫-King <[email protected]>

* deps: upgrade rollup to 2.33.3

Co-authored-by: 信鑫-King <[email protected]>

Co-authored-by: 信鑫-King <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use syntheticNamedExports