Preserve correct `this` for named/default imports #7956

mischnic · 2022-04-14T16:08:32Z

Wraps references to named/default imports with (0, id) like swc/Babel does, so the output is:

    unwrappedNamed: (0, $54cf8bc5675f73b7$export$f7e507eb2d15e768)(),
    unwrappedDefault: (0, $54cf8bc5675f73b7$export$2e2bcd8739ae039)(),
    unwrappedNamespace: $54cf8bc5675f73b7$export$f7e507eb2d15e768(),
    wrappedNamed: (0, $gL1rG.returnThis)(),
    wrappedDefault: (0, $gL1rG.default)(),
    wrappedNamespace: $gL1rG.returnThis()

Problems

I've had to make the packager insert "use strict" to make the test pass. (For the this === ns check).
The added scope hoisting test is failing because for wrapped assets, self-imports of the namespace aren't left alone (should just be module.exports): Fix wrapped assets importing their own namespace #7978

dist/index.js:58
        this === $c332282c8a373e77$exports
                 ^

ReferenceError: $c332282c8a373e77$exports is not defined

mischnic · 2022-04-14T16:28:22Z

packages/transformers/js/core/src/hoist.rs

+      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));
+      }


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.

parcel-benchmark · 2022-04-14T16:49:10Z

Benchmark Results

Kitchen Sink 🚨

Timings

Description	Time	Difference
Cold	FAILED	-0.00ms
Cached	FAILED	-0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

React HackerNews ✅

Timings

Description	Time	Difference
Cold	10.85s	-202.00ms
Cached	506.00ms	+33.00ms ⚠️

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/index.js	486.35kb	-2.46kb 🚀	5.69s	-213.00ms
dist/PermalinkedComment.46b19af5.js	4.21kb	-26.00b 🚀	5.69s	-213.00ms
dist/logo.c5bb83f1.png	246.00b	+0.00b	4.78s	-1.08s 🚀

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/index.js	486.35kb	-2.46kb 🚀	5.69s	-75.00ms
dist/PermalinkedComment.46b19af5.js	4.21kb	-26.00b 🚀	5.69s	-76.00ms

AtlasKit Editor 🚨

Timings

Description	Time	Difference
Cold	FAILED	-0.00ms
Cached	FAILED	-0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Three.js ✅

Timings

Description	Time	Difference
Cold	7.71s	+55.00ms
Cached	307.00ms	+11.00ms

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/Three.js	579.69kb	+368.00b ⚠️	5.55s	+82.00ms

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/Three.js	579.69kb	+368.00b ⚠️	5.61s	+171.00ms

Click here to view a detailed benchmark overview.

devongovett · 2022-04-20T13:23:53Z

Looks like a bunch of tests are failing

parcel-benchmark · 2022-04-20T19:48:33Z

Benchmark Results

Kitchen Sink 🚨

Timings

Description	Time	Difference
Cold	FAILED	-0.00ms
Cached	FAILED	-0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

React HackerNews ✅

Timings

Description	Time	Difference
Cold	10.93s	-50.00ms
Cached	563.00ms	+47.00ms ⚠️

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/index.js	486.35kb	-2.46kb 🚀	5.75s	-11.00ms
dist/PermalinkedComment.46b19af5.js	4.21kb	-26.00b 🚀	5.75s	-11.00ms

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/index.js	486.35kb	-2.46kb 🚀	5.63s	-189.00ms
dist/PermalinkedComment.46b19af5.js	4.21kb	-26.00b 🚀	5.63s	-188.00ms

AtlasKit Editor 🚨

Timings

Description	Time	Difference
Cold	FAILED	-0.00ms
Cached	FAILED	-0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Three.js ✅

Timings

Description	Time	Difference
Cold	7.99s	+157.00ms
Cached	311.00ms	-17.00ms 🚀

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/Three.js	579.69kb	+368.00b ⚠️	5.73s	+126.00ms

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/Three.js	579.69kb	+368.00b ⚠️	5.78s	+147.00ms

Click here to view a detailed benchmark overview.

mischnic · 2022-04-21T22:04:31Z

Looks like a bunch of tests are failing

Because of nodejs/node#38918... I'll probably have to change the fixtures to do global.output = ... instead.

mischnic · 2022-05-13T16:57:37Z

I've removed the change to insert use strict in the packager and instead simply tell run to use strict mode.

mischnic force-pushed the named-import-this branch from b18670d to 67bfa06 Compare April 14, 2022 16:09

mischnic commented Apr 14, 2022

View reviewed changes

devongovett approved these changes Apr 15, 2022

View reviewed changes

mischnic mentioned this pull request Apr 19, 2022

Fix wrapped assets importing their own namespace #7978

Merged

mischnic force-pushed the named-import-this branch from d5224fd to 7b1b9c2 Compare April 20, 2022 19:14

wbinnssmith approved these changes Apr 22, 2022

View reviewed changes

mischnic added 3 commits May 13, 2022 18:56

Add test

0e0fbc9

Adjust this for non-scope-hoisting

78d4702

Adjust this for scope-hoisting

60cc6ec

mischnic force-pushed the named-import-this branch from ba379a6 to 60cc6ec Compare May 13, 2022 16:56

Update existing assertions

c331dc2

devongovett approved these changes May 17, 2022

View reviewed changes

mischnic merged commit a1e9ce5 into v2 May 17, 2022

mischnic deleted the named-import-this branch May 17, 2022 13:06

mischnic added a commit that referenced this pull request May 22, 2022

Adjust tests after #7956

761b8df

mischnic mentioned this pull request May 22, 2022

Fix broken hoist unit tests and run them on CI #8123

Merged

marcins mentioned this pull request Jul 5, 2022

Incorrect code produced with ternaries, imported function calls, and ternaries #8281

Closed

mischnic mentioned this pull request Jan 3, 2023

Imports transformations can break code, including 3rd-party #7125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve correct `this` for named/default imports #7956

Preserve correct `this` for named/default imports #7956

mischnic commented Apr 14, 2022 •

edited

mischnic Apr 14, 2022

parcel-benchmark commented Apr 14, 2022 •

edited

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

devongovett commented Apr 20, 2022

parcel-benchmark commented Apr 20, 2022

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

mischnic commented Apr 21, 2022

mischnic commented May 13, 2022

Preserve correct this for named/default imports #7956

Preserve correct this for named/default imports #7956

Conversation

mischnic commented Apr 14, 2022 • edited

Problems

mischnic Apr 14, 2022

Choose a reason for hiding this comment

parcel-benchmark commented Apr 14, 2022 • edited

Benchmark Results

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

devongovett commented Apr 20, 2022

parcel-benchmark commented Apr 20, 2022

Benchmark Results

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

mischnic commented Apr 21, 2022

mischnic commented May 13, 2022

Preserve correct `this` for named/default imports #7956

Preserve correct `this` for named/default imports #7956

mischnic commented Apr 14, 2022 •

edited

parcel-benchmark commented Apr 14, 2022 •

edited