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

Always respect synthetic namespaces in namespace reexports #3894

Merged
merged 2 commits into from Jan 19, 2021

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Dec 2, 2020

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:
#3857
resolves #3928

Description

This will treat synthetic namespaces as containing "all named exports" also when handling namespace reexports, i.e.

// main.js
import {foo} from './reexport.js';
console.log(foo);

// reexport.js
export * from './synthetic.js';

// synthetic.js
// we assume this is marked as the synthetic namespace
export const __synth = {foo: 'bar'};

should work now.

@rollup-bot
Copy link
Collaborator

rollup-bot commented Dec 2, 2020

Thank you for your contribution! ❤️

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

npm install rollup/rollup#synthetic-reexport

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

@codecov
Copy link

codecov bot commented Dec 2, 2020

Codecov Report

Merging #3894 (8aef56e) into master (5d33573) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3894   +/-   ##
=======================================
  Coverage   97.08%   97.08%           
=======================================
  Files         187      187           
  Lines        6556     6563    +7     
  Branches     1910     1913    +3     
=======================================
+ Hits         6365     6372    +7     
  Misses        101      101           
  Partials       90       90           
Impacted Files Coverage Δ
src/Module.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 5d33573...8aef56e. Read the comment docs.

@Amareis
Copy link
Contributor

Amareis commented Dec 2, 2020

What if reexport from two files with synthetic namespaces? I try to repl it with repl.it, but, unfortunately, I cannot use github version of rollup

npm ERR! prepareGitDep husky > Setting up git hooks
npm ERR! prepareGitDep Husky requires Git >=2.13.0. Got v2.11.0.
npm ERR! prepareGitDep husky > Failed to install

https://repl.it/@Amareis/RemarkableSlipperyRam-2

@lukastaegert
Copy link
Member Author

What if reexport from two files with synthetic namespaces

The first one wins. Which is somewhat correct in so far as the first one already contains all possible exports. Of course this does not take into account that a real runtime would warn/throw in such a scenario if there are conflicting exports in the namespaces or return undefined (cannot remember which, but one of those). But I am inclined to accept that.

@lukastaegert
Copy link
Member Author

So it means if the first export all namespace has syntheticNamedExports, all other ones are ignored. I guess Rollup should warn about this case.

@lukastaegert
Copy link
Member Author

Alternatively we could track synthetic exports and build the resolve order like:

  • real exports
  • synthetic exports from first namespace
  • synthetic export from second namespace

@Amareis
Copy link
Contributor

Amareis commented Dec 2, 2020

I think there is also case about reexport all from external module? It's really simlar cases for rollup.
In real runtime if you import named export from module which reexport all from another modules, this import will be taken from last module, which export it.

@Amareis
Copy link
Contributor

Amareis commented Dec 2, 2020

Does it possible to "merge" this synthetic namespaces at runtime? I.E., instead of

export * from './synthetic1.js';
export * from './synthetic2.js';
export * from './external.js';

will be

import {__moduleExports} from './synthetic1.js';
import {__moduleExports2} from './synthetic2.js';
import * as ext from './external.js';

//this will be marked as synthetic export by rollup itself, optionally will throw on non-existing key access
export const mergedSynthetic = {...__moduleExports, ...__moduleExports2, ...ext} 

@Amareis
Copy link
Contributor

Amareis commented Dec 2, 2020

Also, this opens possibility for external synthetic named exports. Just merge SNE export after external namespace and all is ok!

@lukastaegert
Copy link
Member Author

lukastaegert commented Dec 2, 2020

this import will be taken from last module, which export it

This is not true. Run the following example in a browser in a type="module" script tag:

// index.js
import { foo } from './other.js';
console.log(foo);

// other.js
export * from './a.js';
export * from './b.js';

// a.js
export const foo = 'a';

// b.js
export const foo = 'b';

This example will just throw: The requested module './other.js' contains conflicting star exports for name 'foo'

There is no order between star reexports because in case of conflict, the engine just throws for named imports. If you replace import { foo } from './other.js' with import * as foo from ./other.js then it will not throw but return a namespace that has imports from neither. So if we REALLY want to do this right, we would write a runtime helper that compares exports. While this would be 100% spec compliant, I wonder which users would benefit from this.

@lukastaegert
Copy link
Member Author

lukastaegert commented Dec 2, 2020

Also, this opens possibility for external synthetic named exports

In a manner of speaking, all external imports have synthetic named exports. You have always been able to import what you want from an external import.

@Amareis
Copy link
Contributor

Amareis commented Dec 2, 2020

In a manner of speaking, all external imports have synthetic named exports. You have always been able to import what you want from an external import.

Yep, but still it can be useful for certain use-cases. My interest is simple - I want to make incremental compiling for dev mode, and here external synthetic exports is really needed.

About star reexports it's strange, I thought that I tested this on real runtimes. But maybe I tested it on rollup itself...

@shellscape
Copy link
Contributor

As mentioned in #3928 (comment), I'd give a right arm for something mostly working to unblock the builds I'm attempting to put together. Just ran into this and it's a hard block.

@shellscape
Copy link
Contributor

And @guybedford approves :D

@lukastaegert
Copy link
Member Author

I will have another look tomorrow and see if we can release this. Or maybe slightly improve the logic first.

@lukastaegert
Copy link
Member Author

I slightly changed the logic now: If a file contains namespace reexports from more than one file and the first file has synthetic named exports, then explicit named exports from the second file will have higher precedence than synthetic exports from the first file. While this is still a heuristic, I believe it will usually be closer to user's expectations.
The proper solution, though, would be to add a runtime helper to merge namespaces at runtime instead of compile-time. But this is for another PR.

@lukastaegert lukastaegert merged commit 0ed8722 into master Jan 19, 2021
@lukastaegert lukastaegert deleted the synthetic-reexport branch January 19, 2021 05:13
@shellscape
Copy link
Contributor

Thanks for making this happen!

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.

False Missing Export on export * from
4 participants