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

Use mutable bindings for default exports #4182

Merged
merged 1 commit into from Jul 28, 2021

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Jul 18, 2021

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:
#4168
resolves #2000

Description

This aims to improve handling of default exports and solve many issues listed in #4168.

The most important change is that default exports are now always rendered as

export {binding as default};

instead of

export default binding;

This works much better with how we treat default exports internally (for the most part, not different from named exports) and will potentially allow consumers to avoid creating an additional binding.

Most importantly, this finally allows Rollup to create mutable default exports, something which was not possible before.

Secondly, this detects when we default export a binding in a position where the binding may be in a temporal dead zone and creates an additional variable in these cases so that the error is not swallowed.

Also, an inconsistency is fixed where .default was not written as a computed property.

What is not solved is the main issue listed in #4168 as detecting circular dependency situations is somewhat tricky.

Update: I decided not to add any special handling for using default export mode with a mutable default export yet as I could not decide how to approach this best.

@lukastaegert lukastaegert marked this pull request as draft Jul 18, 2021
@github-actions
Copy link

github-actions bot commented Jul 18, 2021

Thank you for your contribution! ❤️

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

npm install rollup/rollup#improve-default-exports

or load it into the REPL:
https://rollupjs.org/repl/?pr=4182

@codecov
Copy link

codecov bot commented Jul 18, 2021

Codecov Report

Merging #4182 (cc4f9b4) into master (824c53f) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4182      +/-   ##
==========================================
- Coverage   98.34%   98.34%   -0.01%     
==========================================
  Files         202      202              
  Lines        7245     7243       -2     
  Branches     2123     2123              
==========================================
- Hits         7125     7123       -2     
  Misses         58       58              
  Partials       62       62              
Impacted Files Coverage Δ
src/Chunk.ts 100.00% <100.00%> (ø)
src/ast/nodes/Identifier.ts 100.00% <100.00%> (ø)
src/ast/variables/ExportDefaultVariable.ts 100.00% <100.00%> (ø)
src/finalisers/es.ts 100.00% <100.00%> (ø)
src/finalisers/shared/getExportBlock.ts 100.00% <100.00%> (ø)

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 824c53f...cc4f9b4. Read the comment docs.

@guybedford
Copy link
Contributor

guybedford commented Jul 18, 2021

Nice to see. Note this fixes #2000.

@lukastaegert lukastaegert marked this pull request as ready for review Jul 28, 2021
@lukastaegert
Copy link
Member Author

lukastaegert commented Jul 28, 2021

I decided not to add any special handling for using default export mode with a mutable default export yet as I could not decide how to approach this best.

Escape "default" when used as named export
Create new variable for TDZ default exports
@lukastaegert lukastaegert changed the title [WIP] Use mutable bindings for default exports Use mutable bindings for default exports Jul 28, 2021
@lukastaegert lukastaegert merged commit 9a23190 into master Jul 28, 2021
8 of 9 checks passed
@lukastaegert lukastaegert deleted the improve-default-exports branch Jul 28, 2021
@jirutka
Copy link

jirutka commented Dec 28, 2021

This change breaks njs-typescript-starter (#1) because njs (NGINX JavaScript – an alternative JavaScript interpreter) doesn’t support named exports. It seems that this rollup feature is not configurable, so what am I supposed to do?

Input:

function hello () { }
export default { hello }

Output (2.62.0):

function hello() {}
var index = {
  hello
};
export { index as default };

Relevant rollup options:

  output: {
    file: 'index.js',
    format: 'es',
    exports: 'default',
  }

So after the changes introduced in this MR, 'default' in output.exports doesn’t behave as documented in https://rollupjs.org/guide/en/#outputexports.

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

This is not true, this change is a breaking change.

@lukastaegert
Copy link
Member Author

lukastaegert commented Dec 28, 2021

I am sorry, but Rollup only promises to produce ECMA compatible code which it does. There is no spec that I am aware of where you only have some ES 2015 export features but not others,even though it might be breaking for your non-standard-compliant runtime.
If you rely on this, you can always rewrite anything in a renderChunk plugin hook.
If you think there is demand, you could also serve the community by publishing such a plugin.

@jirutka
Copy link

jirutka commented Dec 28, 2021

There is no spec that I am aware of where you only have some ES 2015 export features but not others,even though it might be breaking for your non-standard-compliant runtime.

Yes, that’s true, njs is not fully ECMAScript compliant yet. But I really don’t understand why is Rollup replacing export default { foo } from the source with export { foo as default }, even when output.exports is explicitly set to default, not named. It’s already ES export, so why does Rollup transform it at all? Moreover, the documentation doesn’t mention this surprising behaviour at all.

If you rely on this, you can always rewrite anything in a renderChunk plugin hook.

That’s what I’m trying to do now. However, I was hoping to find a more elegant way than replacing the rendered export using regex.

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Dec 28, 2021

It seems that this rollup feature is not configurable, so what am I supposed to do?

you can bug the njs maintainers to introduce proper named exports. the way it's currently implemented by default exporting an object is pretty weird.

meanwhile, you can pin down the rollup version to the version where the behavior was the same as what you expected.

But I really don’t understand why is Rollup replacing export default { foo } from the source with export { foo as default },

the reason is to be ecma spec compliant.

even when output.exports is explicitly set to default, not named

it's still a default export. just a different syntax.

It’s already ES export, so why does Rollup transform it at all?

rollup is a bundler. there's a bunch of things being transformed in the process.

Pimm added a commit to Pimm/parseHttpDate that referenced this issue Jan 1, 2022
This changes the compiled files in a minor way. See rollup/rollup#4182.
Pimm added a commit to Pimm/mapsort that referenced this issue Jan 1, 2022
This changes the compiled files in a minor way. See rollup/rollup#4182.
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