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: support external star exports on namespace objects #3474

Merged
merged 8 commits into from Apr 4, 2020

Conversation

guybedford
Copy link
Contributor

This replaces the current error with a simple Object.assign expression for the case where a namespace object contains star exports from external modules.

Ideally duplications etc should fail etc, but that seems like overkill - rather the Object.assign just works in the correct order to ensure that local export precedence applies at least. This way valid ES module semantics remain valid, while invalid ES module semantics aren't necessarily caught for the ambiguous cases - the principle SystemJS as a project aims to follow.

Currently the export star searching is only one level deep, so may still have an edge case there - but the implementation carefully throws if that edge case is hit in a way that can be added progressively later.

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:

Description

@guybedford
Copy link
Contributor Author

Not sure what is up with all the extra commas added. Also the first time I committed a lint failure caused all my work to be lost, so I'm now slightly terrified of these git hooks....

@rollup-bot
Copy link
Collaborator

rollup-bot commented Mar 31, 2020

Thank you for your contribution! ❤️

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

npm install rollup/rollup#external-reexport-ns

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

@guybedford
Copy link
Contributor Author

Looks like the 13 failure may be a flake?

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.

Will need to look into this again tomorrow but looks good at first glance. Two minor things regarding tests, especially a form test would be nice here as well.

@codecov
Copy link

codecov bot commented Apr 1, 2020

Codecov Report

Merging #3474 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3474      +/-   ##
==========================================
+ Coverage   95.84%   95.92%   +0.07%     
==========================================
  Files         174      174              
  Lines        5902     5911       +9     
  Branches     1737     1739       +2     
==========================================
+ Hits         5657     5670      +13     
+ Misses        126      124       -2     
+ Partials      119      117       -2     
Impacted Files Coverage Δ
src/Module.ts 98.84% <100.00%> (+0.88%) ⬆️
src/ast/variables/NamespaceVariable.ts 100.00% <100.00%> (ø)
src/ast/variables/ExternalVariable.ts 100.00% <0.00%> (+10.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 722bc25...a390b33. Read the comment docs.

@@ -6,7 +6,7 @@ const d = {
};
const foo = 100;

var ns = /*#__PURE__*/Object.freeze(/*#__PURE__*/Object.assign({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also removed these "double pure comment" cases since I'm not sure that is necessary for the feature? Otherwise seems odd to potentially have three in a row.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately all of them are necessary to have an effect. The annotations only annotate the call itself as pure but not its arguments. I readded the annotations and also slightly simplified the logic and merged it with the synthetic export logic.

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.

Looks good now, I also extended tests so that all cases are covered.

@@ -907,6 +896,19 @@ export default class Module {
}
}

private includeAndGetReexportedExternalNamespaces(): ExternalVariable[] {
Copy link
Member

Choose a reason for hiding this comment

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

I found it a little more straightforward to not push all external namespaces individually to the namespace variable but rather have the context function take care of all that.

...this.reexportedExternalNamespaces.map(variable => variable.getName())
);
}
if (this.syntheticNamedExports) {
Copy link
Member

Choose a reason for hiding this comment

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

It is a little nicer to have synthetic exports in the same assignment

@lukastaegert lukastaegert merged commit 74cec2e into master Apr 4, 2020
@lukastaegert lukastaegert deleted the external-reexport-ns branch April 4, 2020 21:56
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