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

Fix reexporting init-less variable in systemjs #12110

Merged
merged 1 commit into from Sep 28, 2020

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Sep 25, 2020

Q                       A
Fixed Issues? #12091 (comment)
Patch: Bug Fix? Yes
Tests Added + Pass? Updated test fixtures
Documentation PR Link
Any Dependency Changes?
License MIT

This PR checks exportMap when hoisting variables so we don't accidentally push unexported variables to _export call. We also push exported instead of the local variable so it respects cases like

var foo
export { foo as bar }

/cc @guybedford

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories area: modules labels Sep 25, 2020
@babel-bot
Copy link
Collaborator

babel-bot commented Sep 25, 2020

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/28836/

exportNames.push(specifier.exported.name);
exportValues.push(t.cloneNode(specifier.local));
}
// only globals also exported this way
else if (!binding) {

This comment was marked as outdated.

@@ -3,7 +3,7 @@ System.register([], function (_export, _context) {

var foo;

_export("foo", void 0);
_export("bar", void 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -3,7 +3,7 @@ System.register([], function (_export, _context) {

var foo;

_export("foo", void 0);
_export("default", void 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -4,7 +4,7 @@ System.register([], function (_export, _context) {
var foo, bar;

_export({
foo: void 0,
default: void 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 25, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0fe715e:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

nextOdd: nextOdd,
a: void 0
});
_export("nextOdd", nextOdd);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

import { isEven } from "./evens";
export function nextOdd(n) {
return p = isEven(n) ? n + 1 : n + 2;
}
export var p = 5;
for (var a in b) ;
for (var i = 0, j = 0;;) ;
export var isOdd = (function (isEven) {
return function (n) {
return !isEven(n);
};
})(isEven);

@JLHwung JLHwung force-pushed the reexport-initless-variable-systemjs branch from 8bbd608 to 0fe715e Compare September 25, 2020 20:58
JLHwung added a commit to JLHwung/babel that referenced this pull request Sep 25, 2020
JLHwung added a commit to JLHwung/babel that referenced this pull request Sep 25, 2020
@JLHwung JLHwung mentioned this pull request Sep 25, 2020
3 tasks
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @JLHwung for finding these.

@JLHwung JLHwung merged commit 62df8d2 into babel:main Sep 28, 2020
@JLHwung JLHwung deleted the reexport-initless-variable-systemjs branch September 28, 2020 15:38
JLHwung added a commit to JLHwung/babel that referenced this pull request Sep 28, 2020
JLHwung added a commit to JLHwung/babel that referenced this pull request Sep 29, 2020
JLHwung added a commit to JLHwung/babel that referenced this pull request Sep 29, 2020
JLHwung added a commit to JLHwung/babel that referenced this pull request Sep 30, 2020
JLHwung added a commit to JLHwung/babel that referenced this pull request Oct 9, 2020
JLHwung added a commit to JLHwung/babel that referenced this pull request Oct 12, 2020
nicolo-ribaudo pushed a commit that referenced this pull request Oct 14, 2020
nicolo-ribaudo pushed a commit that referenced this pull request Oct 14, 2020
* feat: parse moduleExportName
* feat: add validators
* Support string specifier name in commonjs transform
* Support string specifier name in export-ns-from
* test: add loose testcases
* test: add testcases for amd and umd
* feat: support systemjs
* test: update fixtures fixed in #12110
* add plugin name typings
* test: rename test layout
* feat: implement under moduleStringNames flag
* chore: add plugin syntax module string names
* feat: support ModuleExportName as ModuleExportName
* test: update test fixtures
* fix flow errors
* docs: update AST spec
* feat: support { "some imports" as "some exports" }
* feat: support { "some imports" as "some exports" } in systemjs
* test: add test on `import { "foo" }`
* Address review comments
* add moduleStringNames to missing plugin helpers
* Apply suggestions from code review
* update test fixtures
* Update packages/babel-parser/src/parser/error-message.js
* update test fixtures

Co-Authored-By: Kai Cataldo <kai@kaicataldo.com>
Co-authored-by: Brian Ng <bng412@gmail.com>
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 29, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: modules outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants