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

Warn when implicitly using default export mode #3659

Merged
merged 4 commits into from Jul 18, 2020

Conversation

lukastaegert
Copy link
Member

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:
rollup/plugins#481

Description

This adds a warning when not explicitly specifying a value for output.exports when generating CommonJS output and only a default export is present:

'Entry module "main.js" is implicitly using "default" export mode, which means for CommonJS output that its default export is assigned to "module.exports". For many tools, such CommonJS output will not be interchangeable with the original ES module. If this is intended, explicitly set "output.exports" to either "auto" or "default", otherwise you might want to consider changing the signature of "main.js" to use named exports only.'

As the warning also contains a link to the URL of the output.exports documentation, this one has been extended as well to give a little more context.

This is to encourage moving away from default export mode in CommonJS as it is the source of many interop problems, see rollup/plugins#481

@rollup-bot
Copy link
Collaborator

rollup-bot commented Jul 4, 2020

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#warn-implicit-auto-mode

or load it into the REPL:
https://rollupjs.org/repl/?circleci=12342

@codecov
Copy link

codecov bot commented Jul 4, 2020

Codecov Report

Merging #3659 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3659      +/-   ##
==========================================
+ Coverage   96.71%   96.77%   +0.05%     
==========================================
  Files         183      183              
  Lines        6303     6294       -9     
  Branches     1835     1834       -1     
==========================================
- Hits         6096     6091       -5     
  Misses        105      105              
+ Partials      102       98       -4     
Impacted Files Coverage Δ
src/Chunk.ts 100.00% <ø> (ø)
src/utils/error.ts 100.00% <100.00%> (ø)
src/utils/getExportMode.ts 100.00% <100.00%> (ø)
src/utils/options/normalizeOutputOptions.ts 100.00% <100.00%> (ø)
src/Graph.ts 100.00% <0.00%> (ø)
src/Bundle.ts 100.00% <0.00%> (ø)
src/Module.ts 100.00% <0.00%> (ø)
src/ModuleLoader.ts 100.00% <0.00%> (ø)
src/rollup/rollup.ts 100.00% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ca712d...44b13a2. Read the comment docs.

@lukastaegert lukastaegert merged commit 0fa9758 into master Jul 18, 2020
@lukastaegert lukastaegert deleted the warn-implicit-auto-mode branch July 18, 2020 04:32
@konnextv
Copy link

Just a short question on this behaviour:

I am using Sapper and updated to rollup@2.22.1 - then was confronted with the warning you introduced.
To get rid of the warning I edited rollup.config.js (template) to output: { ...config.client.output(), exports: 'auto' }, for client, server and serviceworker but still get warned that "rollup.config.js" is implicitly using "default" export mode (should this warning even exist for the config itself in the first place?).
Is this possibly some issue with your commit or should I ask about this in the svelte/sapper community?

Thanks in advice and sorry if I am misunderstanding something about this...

@lukastaegert
Copy link
Member Author

There should not be a warning about the config itself, but I do not think Rollup is transpiling the config here but must be called by sapper somehow. If I remember correctly, Rollup CLI is internally using named exports mode for the config, so I guess this must be in how sapper handles things. I would recommend asking there first.

@konnextv
Copy link

Found the problem, sapper indeed uses the API functions rollup.rollup() and bundle.generate(), the option has to be added there.

For anyone coming across this problem, I submitted a Sapper PR: sveltejs/sapper#1331

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