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.

Sign up for GitHub

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

Jump to bottom

Preserve correct this for named/default imports #7956

Merged
merged 4 commits into from May 17, 2022
Merged

Preserve correct this for named/default imports #7956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e0fbc9
Add test
mischnic Apr 14, 2022
78d4702
Adjust this for non-scope-hoisting
mischnic Apr 13, 2022
60cc6ec
Adjust this for scope-hoisting
mischnic May 13, 2022
c331dc2
Update existing assertions
mischnic May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions packages/core/integration-tests/test/integration/js-import-this/index.js
View file
Open in desktop
@@ -0,0 +1,17 @@
import returnThisDefault, { returnThis } from "./other.js";
import * as other from "./other.js";

import returnThisWrappedDefault, { returnThis as returnThisWrapped } from "./other-wrapped.js";
import * as otherWrapped from "./other-wrapped.js";

let result = {
unwrappedNamed: returnThis(),
unwrappedDefault: returnThisDefault(),
unwrappedNamespace: other.returnThis(),
wrappedNamed: returnThisWrapped(),
wrappedDefault: returnThisWrappedDefault(),
wrappedNamespace: otherWrapped.returnThis(),
};

output = result;
export default result;
13 changes: 13 additions & 0 deletions packages/core/integration-tests/test/integration/js-import-this/other-wrapped.js
View file
Open in desktop
@@ -0,0 +1,13 @@
import * as ns from "./other-wrapped.js";

let y = typeof module !== "undefined" ? module : {};

export function returnThis() {
if (y != null) {
return [this === undefined, this === ns];
} else {
throw new Error();
}
}

export default returnThis;
7 changes: 7 additions & 0 deletions packages/core/integration-tests/test/integration/js-import-this/other.js
View file
Open in desktop
@@ -0,0 +1,7 @@
import * as ns from "./other.js";

export function returnThis() {
return [this === undefined, this === ns];
}

export default returnThis;
15 changes: 15 additions & 0 deletions packages/core/integration-tests/test/javascript.js
View file
Open in desktop
Expand Up @@ -5267,6 +5267,21 @@ describe('javascript', function () {
assert.deepEqual(res.default, 'x: 123');
});

it('should call named imports without this context', async function () {
let b = await bundle(
path.join(__dirname, 'integration/js-import-this/index.js'),
);
let res = await run(b, {output: null}, {strict: true});
assert.deepEqual(res.default, {
unwrappedNamed: [true, false],
unwrappedDefault: [true, false],
unwrappedNamespace: [false, true],
wrappedNamed: [true, false],
wrappedDefault: [true, false],
wrappedNamespace: [false, true],
});
});

it('should only replace free references to require', async () => {
let b = await bundle(
path.join(__dirname, 'integration/js-require-free/index.js'),
Expand Down
2 changes: 1 addition & 1 deletion packages/core/integration-tests/test/output-formats.js
View file
Open in desktop
Expand Up @@ -191,7 +191,7 @@ describe('output formats', function () {

let dist = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
assert(dist.includes('= require("lodash")'));
assert(dist.includes('= ($parcel$interopDefault('));
assert(dist.includes('= (0, ($parcel$interopDefault('));
assert(/var {add: \s*\$.+?\$add\s*} = lodash/);
assert.equal((await run(b)).bar, 6);
});
Expand Down
17 changes: 17 additions & 0 deletions packages/core/integration-tests/test/scope-hoisting.js
View file
Open in desktop
Expand Up @@ -5284,6 +5284,23 @@ describe('scope hoisting', function () {
assert.deepEqual(res, 'x: 123');
});

it('should call named imports without this context', async function () {
let b = await bundle(
path.join(__dirname, 'integration/js-import-this/index.js'),
);
let res = await run(b, {output: null}, {strict: true});
assert.deepEqual(res, {
unwrappedNamed: [true, false],
unwrappedDefault: [true, false],
// TODO: unwrappedNamespace should actually be `[false, true]` but we optimize
// the `ns.foo` expression into a named import, so that namespace isn't available anymore.
unwrappedNamespace: [true, false],
wrappedNamed: [true, false],
wrappedDefault: [true, false],
wrappedNamespace: [false, true],
});
});

it('should insert the prelude for sibling bundles referenced in HTML', async function () {
let b = await bundle(
path.join(
Expand Down
18 changes: 10 additions & 8 deletions packages/core/integration-tests/test/transpilation.js
View file
Open in desktop
Expand Up @@ -121,7 +121,9 @@ describe('transpilation', function () {
path.join(distDir, 'pure-comment.js'),
'utf8',
);
assert(file.includes('/*#__PURE__*/ _reactDefault.default.createElement'));
assert(
file.includes('/*#__PURE__*/ (0, _reactDefault.default).createElement'),
);

let res = await run(b);
assert(res.Foo());
Expand Down Expand Up @@ -190,7 +192,7 @@ describe('transpilation', function () {

let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
assert(file.includes('react/jsx-dev-runtime'));
assert(file.includes('_jsxDevRuntime.jsxDEV("div"'));
assert(file.includes('(0, _jsxDevRuntime.jsxDEV)("div"'));
});

it('should support the automatic JSX runtime with preact >= 10.5', async function () {
Expand All @@ -200,7 +202,7 @@ describe('transpilation', function () {

let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
assert(file.includes('preact/jsx-dev-runtime'));
assert(file.includes('_jsxDevRuntime.jsxDEV("div"'));
assert(file.includes('(0, _jsxDevRuntime.jsxDEV)("div"'));
});

it('should support the automatic JSX runtime with React ^16.14.0', async function () {
Expand All @@ -210,7 +212,7 @@ describe('transpilation', function () {

let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
assert(file.includes('react/jsx-dev-runtime'));
assert(file.includes('_jsxDevRuntime.jsxDEV("div"'));
assert(file.includes('(0, _jsxDevRuntime.jsxDEV)("div"'));
});

it('should support the automatic JSX runtime with React 18 prereleases', async function () {
Expand All @@ -220,7 +222,7 @@ describe('transpilation', function () {

let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
assert(file.includes('react/jsx-dev-runtime'));
assert(file.includes('_jsxDevRuntime.jsxDEV("div"'));
assert(file.includes('(0, _jsxDevRuntime.jsxDEV)("div"'));
});

it('should support the automatic JSX runtime with experimental React versions', async function () {
Expand All @@ -230,7 +232,7 @@ describe('transpilation', function () {

let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
assert(file.includes('react/jsx-dev-runtime'));
assert(file.includes('_jsxDevRuntime.jsxDEV("div"'));
assert(file.includes('(0, _jsxDevRuntime.jsxDEV)("div"'));
});

it('should support the automatic JSX runtime with preact with alias', async function () {
Expand All @@ -243,7 +245,7 @@ describe('transpilation', function () {

let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
assert(/\Wreact\/jsx-dev-runtime\W/.test(file));
assert(file.includes('_jsxDevRuntime.jsxDEV("div"'));
assert(file.includes('(0, _jsxDevRuntime.jsxDEV)("div"'));
});

it('should support the automatic JSX runtime with explicit tsconfig.json', async function () {
Expand All @@ -253,7 +255,7 @@ describe('transpilation', function () {

let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
assert(file.includes('preact/jsx-dev-runtime'));
assert(file.includes('_jsxDevRuntime.jsxDEV("div"'));
assert(file.includes('(0, _jsxDevRuntime.jsxDEV)("div"'));
});

it('should support explicit JSX pragma in tsconfig.json', async function () {
Expand Down
11 changes: 11 additions & 0 deletions packages/transformers/js/core/src/hoist.rs
View file
Open in desktop
Expand Up @@ -668,6 +668,17 @@ impl<'a> Fold for Hoist<'a> {
}
}
}
Expr::Ident(ident) => {
if let Some(Import { specifier, .. }) = self.collect.imports.get(&id!(ident)) {
if specifier != "*" {
return Expr::Seq(SeqExpr {
span: ident.span,
exprs: vec![0.into(), Box::new(Expr::Ident(ident.fold_with(self)))],
});
}
}
return Expr::Ident(ident.fold_with(self));
}
Comment on lines +671 to +681
Copy link
Member Author

Choose a reason for hiding this comment

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

A cleaner fix (and what I tried to do first) was to move the fold_ident function into this match. But fold_ident runs not only referenced identifiers (=Expr::Ident) but also the names of function/variable declarations.
I'm not sure if it would make sense to split up fold_ident/extract the parts we need and use them here directly instead.

_ => {}
}

Expand Down
12 changes: 9 additions & 3 deletions packages/transformers/js/core/src/modules.rs
View file
Open in desktop
Expand Up @@ -230,9 +230,15 @@ impl ESMFold {
self.get_require_name(source, DUMMY_SP)
};

Expr::Member(MemberExpr {
obj: Box::new(Expr::Ident(obj)),
prop: MemberProp::Ident(Ident::new(imported.clone(), DUMMY_SP)),
Expr::Seq(SeqExpr {
exprs: vec![
0.into(),
Box::new(Expr::Member(MemberExpr {
obj: Box::new(Expr::Ident(obj)),
prop: MemberProp::Ident(Ident::new(imported.clone(), DUMMY_SP)),
span,
})),
],
span,
})
}
Expand Down