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

Preserve correct this for named/default imports #7956

Merged
merged 4 commits into from
May 17, 2022
Merged

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Apr 14, 2022

Closes #7920

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

  1. I've had to make the packager insert "use strict" to make the test pass. (For the this === ns check).
  2. 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

Comment on lines +671 to +681
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));
}
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.

@parcel-benchmark
Copy link

parcel-benchmark commented Apr 14, 2022

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
Copy link
Member

Looks like a bunch of tests are failing

@parcel-benchmark
Copy link

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
Copy link
Member Author

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
Copy link
Member Author

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

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.

Wrapping modules changes invocation of exported functions
4 participants