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
Conversation
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.... |
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via
or load it into the REPL: |
Looks like the 13 failure may be a flake? |
There was a problem hiding this 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.
test/function/samples/internal-reexports-from-external/_config.js
Outdated
Show resolved
Hide resolved
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
f2b8ee6
to
751113b
Compare
751113b
to
5f4310d
Compare
@@ -6,7 +6,7 @@ const d = { | |||
}; | |||
const foo = 100; | |||
|
|||
var ns = /*#__PURE__*/Object.freeze(/*#__PURE__*/Object.assign({ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
50730cf
to
8ff2966
Compare
There was a problem hiding this 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[] { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
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:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description