From ea9ac58ddf0c8d0410adab467bc6c84ab726dddd Mon Sep 17 00:00:00 2001 From: achan3 Date: Wed, 4 Oct 2023 16:57:51 -0400 Subject: [PATCH 01/20] recognize 'this' as a potential keyword linking to an export --- packages/transformers/js/core/src/collect.rs | 18 ++++++++++ packages/transformers/js/core/src/hoist.rs | 35 +++++++++++++++++++- packages/transformers/js/core/src/utils.rs | 5 +++ 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/packages/transformers/js/core/src/collect.rs b/packages/transformers/js/core/src/collect.rs index 3611b906e2d..3f273c45e87 100644 --- a/packages/transformers/js/core/src/collect.rs +++ b/packages/transformers/js/core/src/collect.rs @@ -58,6 +58,7 @@ pub struct Collect { pub should_wrap: bool, /// local variable binding -> descriptor pub imports: HashMap, + pub thises: HashMap, /// exported name -> descriptor pub exports: HashMap, /// local variable binding -> exported name @@ -129,6 +130,7 @@ impl Collect { is_esm: false, should_wrap: false, imports: HashMap::new(), + thises: HashMap::new(), exports: HashMap::new(), exports_locals: HashMap::new(), exports_all: HashMap::new(), @@ -255,6 +257,18 @@ impl Visit for Collect { } } + for (key, node) in &self.thises { + if let MemberProp::Ident(prop) = &node.prop { + if self.exports.contains_key(&prop.sym) { + self.should_wrap = true; + bailouts.push(Bailout { + loc: SourceLocation::from(&self.source_map, node.span), + reason: BailoutReason::ThisInExport, + }) + } + } + } + bailouts.sort_by(|a, b| a.loc.partial_cmp(&b.loc).unwrap()); } } @@ -681,6 +695,10 @@ impl Visit for Collect { Expr::This(_this) => { if self.in_module_this { handle_export!(); + } else { + if let MemberProp::Ident(prop) = &node.prop { + self.thises.insert(id!(prop), node.clone()); + } } return; } diff --git a/packages/transformers/js/core/src/hoist.rs b/packages/transformers/js/core/src/hoist.rs index 32ed187688f..73ec9859bd4 100644 --- a/packages/transformers/js/core/src/hoist.rs +++ b/packages/transformers/js/core/src/hoist.rs @@ -34,7 +34,7 @@ pub fn hoist( ) -> Result<(Module, HoistResult, Vec), Vec> { let mut hoist = Hoist::new(module_id, unresolved_mark, collect); let module = module.fold_with(&mut hoist); - + println!("here in hoist"); if !hoist.diagnostics.is_empty() { return Err(hoist.diagnostics); } @@ -624,6 +624,7 @@ impl<'a> Fold for Hoist<'a> { && self.collect.static_cjs_exports && !self.collect.should_wrap { + println!("still scope hoisting"); self.self_references.insert(key.clone()); return Expr::Ident(self.get_export_ident(member.span, &key)); } @@ -3407,4 +3408,36 @@ mod tests { } ); } + + #[test] + fn collect_used_local_exports() { + let (collect, code, _hoist) = parse( + r#" + exports.foo = function() { + exports.bar() + } + + exports.bar = function() { + this.baz() + } + + exports.baz = function() { + return 2 + } + + console.log(exports.bar) + "#, + ); + assert_eq!( + collect + .bailouts + .unwrap() + .iter() + .map(|b| &b.reason) + .collect::>(), + vec![&BailoutReason::ThisInExport] + ); + assert_eq!(collect.should_wrap, true); + println!("{}", code); + } } diff --git a/packages/transformers/js/core/src/utils.rs b/packages/transformers/js/core/src/utils.rs index bdab53c1873..ecf1d8e01f0 100644 --- a/packages/transformers/js/core/src/utils.rs +++ b/packages/transformers/js/core/src/utils.rs @@ -306,6 +306,7 @@ pub enum BailoutReason { ModuleReassignment, NonStaticDynamicImport, NonStaticAccess, + ThisInExport, } impl BailoutReason { @@ -355,6 +356,10 @@ impl BailoutReason { "Non-static access of an `import` or `require`. This causes tree shaking to be disabled for the resolved module.", "https://parceljs.org/features/scope-hoisting/#dynamic-member-accesses" ), + BailoutReason::ThisInExport => ( + "ThisInExport placeholder", + "https://parceljs.org/features/scope-hoisting/#dynamic-member-accesses" + ), } } } From 962fb9439b12320b0f30245dd6fc9b7d986eb4ee Mon Sep 17 00:00:00 2001 From: achan3 Date: Wed, 4 Oct 2023 17:10:17 -0400 Subject: [PATCH 02/20] Add co-authors. Co-authored-by: Eric Eldredge Co-authored-by: Brian Tedder --- packages/transformers/js/core/src/hoist.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/transformers/js/core/src/hoist.rs b/packages/transformers/js/core/src/hoist.rs index 73ec9859bd4..c170cd69c6c 100644 --- a/packages/transformers/js/core/src/hoist.rs +++ b/packages/transformers/js/core/src/hoist.rs @@ -3438,6 +3438,7 @@ mod tests { vec![&BailoutReason::ThisInExport] ); assert_eq!(collect.should_wrap, true); + println!("{}", code); } } From 0b286b18729eb7a742b9830479aa1ccca2ef5bb2 Mon Sep 17 00:00:00 2001 From: Brian Tedder Date: Wed, 4 Oct 2023 17:09:58 -0500 Subject: [PATCH 03/20] update BailoutReason text, fix test name --- packages/transformers/js/core/src/collect.rs | 2 +- packages/transformers/js/core/src/hoist.rs | 8 +++----- packages/transformers/js/core/src/utils.rs | 4 ++-- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/packages/transformers/js/core/src/collect.rs b/packages/transformers/js/core/src/collect.rs index 3f273c45e87..e7582624104 100644 --- a/packages/transformers/js/core/src/collect.rs +++ b/packages/transformers/js/core/src/collect.rs @@ -257,7 +257,7 @@ impl Visit for Collect { } } - for (key, node) in &self.thises { + for (_key, node) in &self.thises { if let MemberProp::Ident(prop) = &node.prop { if self.exports.contains_key(&prop.sym) { self.should_wrap = true; diff --git a/packages/transformers/js/core/src/hoist.rs b/packages/transformers/js/core/src/hoist.rs index c170cd69c6c..4f17b4e8a2c 100644 --- a/packages/transformers/js/core/src/hoist.rs +++ b/packages/transformers/js/core/src/hoist.rs @@ -3410,8 +3410,8 @@ mod tests { } #[test] - fn collect_used_local_exports() { - let (collect, code, _hoist) = parse( + fn collect_this_exports() { + let (collect, _code, _hoist) = parse( r#" exports.foo = function() { exports.bar() @@ -3424,7 +3424,7 @@ mod tests { exports.baz = function() { return 2 } - + console.log(exports.bar) "#, ); @@ -3438,7 +3438,5 @@ mod tests { vec![&BailoutReason::ThisInExport] ); assert_eq!(collect.should_wrap, true); - - println!("{}", code); } } diff --git a/packages/transformers/js/core/src/utils.rs b/packages/transformers/js/core/src/utils.rs index ecf1d8e01f0..cb53bee5c60 100644 --- a/packages/transformers/js/core/src/utils.rs +++ b/packages/transformers/js/core/src/utils.rs @@ -357,8 +357,8 @@ impl BailoutReason { "https://parceljs.org/features/scope-hoisting/#dynamic-member-accesses" ), BailoutReason::ThisInExport => ( - "ThisInExport placeholder", - "https://parceljs.org/features/scope-hoisting/#dynamic-member-accesses" + "Module contains `this` access of an exported value. This causes the module to be wrapped and tree-shaking to be disabled.", + "" ), } } From f25069d04b53fece77c820d9b5188469036088cd Mon Sep 17 00:00:00 2001 From: Brian Tedder Date: Wed, 4 Oct 2023 17:12:14 -0500 Subject: [PATCH 04/20] rename "thises" to "this_exprs" --- packages/transformers/js/core/src/collect.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/transformers/js/core/src/collect.rs b/packages/transformers/js/core/src/collect.rs index e7582624104..bcf42bbc26b 100644 --- a/packages/transformers/js/core/src/collect.rs +++ b/packages/transformers/js/core/src/collect.rs @@ -58,7 +58,7 @@ pub struct Collect { pub should_wrap: bool, /// local variable binding -> descriptor pub imports: HashMap, - pub thises: HashMap, + pub this_exprs: HashMap, /// exported name -> descriptor pub exports: HashMap, /// local variable binding -> exported name @@ -130,7 +130,7 @@ impl Collect { is_esm: false, should_wrap: false, imports: HashMap::new(), - thises: HashMap::new(), + this_exprs: HashMap::new(), exports: HashMap::new(), exports_locals: HashMap::new(), exports_all: HashMap::new(), @@ -257,7 +257,7 @@ impl Visit for Collect { } } - for (_key, node) in &self.thises { + for (_key, node) in &self.this_exprs { if let MemberProp::Ident(prop) = &node.prop { if self.exports.contains_key(&prop.sym) { self.should_wrap = true; @@ -697,7 +697,7 @@ impl Visit for Collect { handle_export!(); } else { if let MemberProp::Ident(prop) = &node.prop { - self.thises.insert(id!(prop), node.clone()); + self.this_exprs.insert(id!(prop), node.clone()); } } return; From 053a3e3ccb2c0f1d802613ebd14c3d7626195104 Mon Sep 17 00:00:00 2001 From: Brian Tedder Date: Wed, 4 Oct 2023 17:14:26 -0500 Subject: [PATCH 05/20] remove print statements --- packages/transformers/js/core/src/hoist.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/transformers/js/core/src/hoist.rs b/packages/transformers/js/core/src/hoist.rs index 4f17b4e8a2c..095c6030cf6 100644 --- a/packages/transformers/js/core/src/hoist.rs +++ b/packages/transformers/js/core/src/hoist.rs @@ -34,7 +34,6 @@ pub fn hoist( ) -> Result<(Module, HoistResult, Vec), Vec> { let mut hoist = Hoist::new(module_id, unresolved_mark, collect); let module = module.fold_with(&mut hoist); - println!("here in hoist"); if !hoist.diagnostics.is_empty() { return Err(hoist.diagnostics); } @@ -624,7 +623,6 @@ impl<'a> Fold for Hoist<'a> { && self.collect.static_cjs_exports && !self.collect.should_wrap { - println!("still scope hoisting"); self.self_references.insert(key.clone()); return Expr::Ident(self.get_export_ident(member.span, &key)); } From d8df4be4faa5b6f3fc91f669b29463f48ff250ad Mon Sep 17 00:00:00 2001 From: Brian Tedder Date: Wed, 4 Oct 2023 17:16:04 -0500 Subject: [PATCH 06/20] cleanup --- packages/transformers/js/core/src/hoist.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/transformers/js/core/src/hoist.rs b/packages/transformers/js/core/src/hoist.rs index 095c6030cf6..76f999646ad 100644 --- a/packages/transformers/js/core/src/hoist.rs +++ b/packages/transformers/js/core/src/hoist.rs @@ -34,6 +34,7 @@ pub fn hoist( ) -> Result<(Module, HoistResult, Vec), Vec> { let mut hoist = Hoist::new(module_id, unresolved_mark, collect); let module = module.fold_with(&mut hoist); + if !hoist.diagnostics.is_empty() { return Err(hoist.diagnostics); } From 649f8d9ec09d3e76c0b52b50ebf95c87030d248d Mon Sep 17 00:00:00 2001 From: Brian Tedder Date: Wed, 4 Oct 2023 17:47:20 -0500 Subject: [PATCH 07/20] don't wrap when `this` collides in class --- packages/transformers/js/core/src/collect.rs | 20 +++++++++-- packages/transformers/js/core/src/hoist.rs | 35 ++++++++++++++++++-- 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/packages/transformers/js/core/src/collect.rs b/packages/transformers/js/core/src/collect.rs index bcf42bbc26b..663a6f2ada9 100644 --- a/packages/transformers/js/core/src/collect.rs +++ b/packages/transformers/js/core/src/collect.rs @@ -77,6 +77,7 @@ pub struct Collect { in_export_decl: bool, in_function: bool, in_assign: bool, + in_class: bool, } #[derive(Debug, Serialize)] @@ -144,6 +145,7 @@ impl Collect { in_export_decl: false, in_function: false, in_assign: false, + in_class: false, bailouts: if trace_bailouts { Some(vec![]) } else { None }, } } @@ -274,7 +276,6 @@ impl Visit for Collect { } collect_visit_fn!(visit_function, Function); - collect_visit_fn!(visit_class, Class); collect_visit_fn!(visit_getter_prop, GetterProp); collect_visit_fn!(visit_setter_prop, SetterProp); @@ -695,7 +696,7 @@ impl Visit for Collect { Expr::This(_this) => { if self.in_module_this { handle_export!(); - } else { + } else if !self.in_class { if let MemberProp::Ident(prop) = &node.prop { self.this_exprs.insert(id!(prop), node.clone()); } @@ -787,6 +788,21 @@ impl Visit for Collect { } } + fn visit_class(&mut self, class: &Class) { + let in_module_this = self.in_module_this; + let in_function = self.in_function; + let in_class = self.in_class; + + self.in_module_this = false; + self.in_function = true; + self.in_class = true; + + class.visit_children_with(self); + self.in_module_this = in_module_this; + self.in_function = in_function; + self.in_class = in_class; + } + fn visit_this_expr(&mut self, node: &ThisExpr) { if self.in_module_this { self.has_cjs_exports = true; diff --git a/packages/transformers/js/core/src/hoist.rs b/packages/transformers/js/core/src/hoist.rs index 76f999646ad..da0108eda26 100644 --- a/packages/transformers/js/core/src/hoist.rs +++ b/packages/transformers/js/core/src/hoist.rs @@ -3410,6 +3410,7 @@ mod tests { #[test] fn collect_this_exports() { + // module is wrapped when `this` accessor matches an export let (collect, _code, _hoist) = parse( r#" exports.foo = function() { @@ -3423,8 +3424,6 @@ mod tests { exports.baz = function() { return 2 } - - console.log(exports.bar) "#, ); assert_eq!( @@ -3437,5 +3436,37 @@ mod tests { vec![&BailoutReason::ThisInExport] ); assert_eq!(collect.should_wrap, true); + + // module is not wrapped when `this` inside a class collides with an export + let (collect, _code, _hoist) = parse( + r#" + class Foo { + constructor() { + this.a = 4 + } + + bar() { + return this.baz() + } + + baz() { + return this.a + } + } + + exports.baz = new Foo() + exports.a = 2 + "#, + ); + assert_eq!( + collect + .bailouts + .unwrap() + .iter() + .map(|b| &b.reason) + .collect::>(), + Vec::<&BailoutReason>::new() + ); + assert_eq!(collect.should_wrap, false); } } From 41676e002e55cf7d0cd34d47749165ba8fb52c14 Mon Sep 17 00:00:00 2001 From: achan3 Date: Thu, 5 Oct 2023 15:41:06 -0400 Subject: [PATCH 08/20] move bailouts outside of tracing --- packages/transformers/js/core/src/collect.rs | 21 +++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/packages/transformers/js/core/src/collect.rs b/packages/transformers/js/core/src/collect.rs index 663a6f2ada9..fabbb0710dc 100644 --- a/packages/transformers/js/core/src/collect.rs +++ b/packages/transformers/js/core/src/collect.rs @@ -245,6 +245,15 @@ impl Visit for Collect { } self.in_module_this = false; + for (_key, node) in self.this_exprs.clone() { + if let MemberProp::Ident(prop) = &node.prop { + if self.exports.contains_key(&prop.sym) { + self.should_wrap = true; + self.add_bailout(node.span, BailoutReason::ThisInExport); + } + } + } + if let Some(bailouts) = &mut self.bailouts { for (key, Import { specifier, .. }) in &self.imports { if specifier == "*" { @@ -259,18 +268,6 @@ impl Visit for Collect { } } - for (_key, node) in &self.this_exprs { - if let MemberProp::Ident(prop) = &node.prop { - if self.exports.contains_key(&prop.sym) { - self.should_wrap = true; - bailouts.push(Bailout { - loc: SourceLocation::from(&self.source_map, node.span), - reason: BailoutReason::ThisInExport, - }) - } - } - } - bailouts.sort_by(|a, b| a.loc.partial_cmp(&b.loc).unwrap()); } } From fd15591cc9a2f8bf9954beb32a697445121129f0 Mon Sep 17 00:00:00 2001 From: achan3 Date: Thu, 5 Oct 2023 17:15:07 -0400 Subject: [PATCH 09/20] add integration test --- .../test/integration/exports-this/a.js | 11 ++++++++++ .../integration-tests/test/scope-hoisting.js | 22 +++++++++++++++++++ 2 files changed, 33 insertions(+) create mode 100644 packages/core/integration-tests/test/integration/exports-this/a.js diff --git a/packages/core/integration-tests/test/integration/exports-this/a.js b/packages/core/integration-tests/test/integration/exports-this/a.js new file mode 100644 index 00000000000..846721fe5bf --- /dev/null +++ b/packages/core/integration-tests/test/integration/exports-this/a.js @@ -0,0 +1,11 @@ +exports.foo = function() { + exports.bar() +} + +exports.bar = function() { + this.baz() +} + +exports.baz = function() { + return 2 +} \ No newline at end of file diff --git a/packages/core/integration-tests/test/scope-hoisting.js b/packages/core/integration-tests/test/scope-hoisting.js index a23e7608444..57b8e6743d2 100644 --- a/packages/core/integration-tests/test/scope-hoisting.js +++ b/packages/core/integration-tests/test/scope-hoisting.js @@ -1,6 +1,7 @@ import assert from 'assert'; import path from 'path'; import nullthrows from 'nullthrows'; +import {readFile} from 'fs'; import {normalizePath} from '@parcel/utils'; import {createWorkerFarm} from '@parcel/core'; import {md} from '@parcel/diagnostic'; @@ -51,6 +52,27 @@ const bundler = (name, opts = {}) => { describe('scope hoisting', function () { describe('es6', function () { + it.only('should wrap when this could refer to an export', async function () { + let b = await bundle( + path.join(__dirname, '/integration/exports-this/a.js'), + { + mode: 'production', + defaultTargetOptions: { + shouldScopeHoist: true, + shouldOptimize: false, + }, + }, + ); + + let contents = await outputFS.readFile( + b.getBundles().find(b => /a\.js/.test(b.filePath)).filePath, + 'utf8', + ); + + let exports_found = contents.includes('$exports$'); + assert.equal(exports_found, false); + }); + it('supports default imports and exports of expressions', async function () { let b = await bundle( path.join( From 844bbd61bfa49ad43912a27197ab49e8584fa732 Mon Sep 17 00:00:00 2001 From: achan3 Date: Thu, 5 Oct 2023 17:16:43 -0400 Subject: [PATCH 10/20] cleanup integration test --- packages/core/integration-tests/test/scope-hoisting.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/core/integration-tests/test/scope-hoisting.js b/packages/core/integration-tests/test/scope-hoisting.js index 57b8e6743d2..99a73cae3de 100644 --- a/packages/core/integration-tests/test/scope-hoisting.js +++ b/packages/core/integration-tests/test/scope-hoisting.js @@ -1,7 +1,6 @@ import assert from 'assert'; import path from 'path'; import nullthrows from 'nullthrows'; -import {readFile} from 'fs'; import {normalizePath} from '@parcel/utils'; import {createWorkerFarm} from '@parcel/core'; import {md} from '@parcel/diagnostic'; @@ -52,7 +51,7 @@ const bundler = (name, opts = {}) => { describe('scope hoisting', function () { describe('es6', function () { - it.only('should wrap when this could refer to an export', async function () { + it('should wrap when this could refer to an export', async function () { let b = await bundle( path.join(__dirname, '/integration/exports-this/a.js'), { From 93d566dfd75dac2804a09bf9011b4f0cd6ddcccc Mon Sep 17 00:00:00 2001 From: achan3 Date: Fri, 6 Oct 2023 11:07:58 -0400 Subject: [PATCH 11/20] update test: move to commonjs and change search --- .../integration-tests/test/scope-hoisting.js | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/packages/core/integration-tests/test/scope-hoisting.js b/packages/core/integration-tests/test/scope-hoisting.js index 99a73cae3de..e5e98cc0b61 100644 --- a/packages/core/integration-tests/test/scope-hoisting.js +++ b/packages/core/integration-tests/test/scope-hoisting.js @@ -51,27 +51,6 @@ const bundler = (name, opts = {}) => { describe('scope hoisting', function () { describe('es6', function () { - it('should wrap when this could refer to an export', async function () { - let b = await bundle( - path.join(__dirname, '/integration/exports-this/a.js'), - { - mode: 'production', - defaultTargetOptions: { - shouldScopeHoist: true, - shouldOptimize: false, - }, - }, - ); - - let contents = await outputFS.readFile( - b.getBundles().find(b => /a\.js/.test(b.filePath)).filePath, - 'utf8', - ); - - let exports_found = contents.includes('$exports$'); - assert.equal(exports_found, false); - }); - it('supports default imports and exports of expressions', async function () { let b = await bundle( path.join( @@ -3595,6 +3574,27 @@ describe('scope hoisting', function () { }); describe('commonjs', function () { + it('should wrap when this could refer to an export', async function () { + let b = await bundle( + path.join(__dirname, '/integration/exports-this/a.js'), + { + mode: 'production', + defaultTargetOptions: { + shouldScopeHoist: true, + shouldOptimize: false, + }, + }, + ); + + let contents = await outputFS.readFile( + b.getBundles().find(b => /a\.js/.test(b.filePath)).filePath, + 'utf8', + ); + + let exports_found = contents.includes('exports.bar()'); + assert.equal(exports_found, true); + }); + it('supports require of commonjs modules', async function () { let b = await bundle( path.join( From df4a1332bf4d31a739a018de44995e7294636d4c Mon Sep 17 00:00:00 2001 From: achan3 Date: Fri, 6 Oct 2023 11:10:16 -0400 Subject: [PATCH 12/20] update naming --- packages/core/integration-tests/test/scope-hoisting.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/integration-tests/test/scope-hoisting.js b/packages/core/integration-tests/test/scope-hoisting.js index e5e98cc0b61..9169d5f099d 100644 --- a/packages/core/integration-tests/test/scope-hoisting.js +++ b/packages/core/integration-tests/test/scope-hoisting.js @@ -3591,8 +3591,8 @@ describe('scope hoisting', function () { 'utf8', ); - let exports_found = contents.includes('exports.bar()'); - assert.equal(exports_found, true); + let wrapped = contents.includes('exports.bar()'); + assert.equal(wrapped, true); }); it('supports require of commonjs modules', async function () { From c22d90006b69d9d221e967a13a75e8a720bd3919 Mon Sep 17 00:00:00 2001 From: adelchan07 <66882888+adelchan07@users.noreply.github.com> Date: Fri, 6 Oct 2023 12:53:08 -0400 Subject: [PATCH 13/20] Update packages/core/integration-tests/test/scope-hoisting.js Co-authored-by: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com> --- packages/core/integration-tests/test/scope-hoisting.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/core/integration-tests/test/scope-hoisting.js b/packages/core/integration-tests/test/scope-hoisting.js index 9169d5f099d..f2093fac06a 100644 --- a/packages/core/integration-tests/test/scope-hoisting.js +++ b/packages/core/integration-tests/test/scope-hoisting.js @@ -3577,17 +3577,10 @@ describe('scope hoisting', function () { it('should wrap when this could refer to an export', async function () { let b = await bundle( path.join(__dirname, '/integration/exports-this/a.js'), - { - mode: 'production', - defaultTargetOptions: { - shouldScopeHoist: true, - shouldOptimize: false, - }, - }, ); let contents = await outputFS.readFile( - b.getBundles().find(b => /a\.js/.test(b.filePath)).filePath, + b.getBundles()[0].filePath, 'utf8', ); From bec3225d98ed52f704784091fcce6fd45ed02eac Mon Sep 17 00:00:00 2001 From: achan3 Date: Fri, 6 Oct 2023 13:13:39 -0400 Subject: [PATCH 14/20] move fixture for test into commonjs folder --- .../{ => scope-hoisting/commonjs}/exports-this/a.js | 0 packages/core/integration-tests/test/scope-hoisting.js | 5 ++++- 2 files changed, 4 insertions(+), 1 deletion(-) rename packages/core/integration-tests/test/integration/{ => scope-hoisting/commonjs}/exports-this/a.js (100%) diff --git a/packages/core/integration-tests/test/integration/exports-this/a.js b/packages/core/integration-tests/test/integration/scope-hoisting/commonjs/exports-this/a.js similarity index 100% rename from packages/core/integration-tests/test/integration/exports-this/a.js rename to packages/core/integration-tests/test/integration/scope-hoisting/commonjs/exports-this/a.js diff --git a/packages/core/integration-tests/test/scope-hoisting.js b/packages/core/integration-tests/test/scope-hoisting.js index f2093fac06a..5e7b871432f 100644 --- a/packages/core/integration-tests/test/scope-hoisting.js +++ b/packages/core/integration-tests/test/scope-hoisting.js @@ -3576,7 +3576,10 @@ describe('scope hoisting', function () { describe('commonjs', function () { it('should wrap when this could refer to an export', async function () { let b = await bundle( - path.join(__dirname, '/integration/exports-this/a.js'), + path.join( + __dirname, + '/integration/scope-hoisting/commonjs/exports-this/a.js', + ), ); let contents = await outputFS.readFile( From d677c71dd106b75b7199751403603b06b2ae22da Mon Sep 17 00:00:00 2001 From: achan3 Date: Fri, 6 Oct 2023 14:28:02 -0400 Subject: [PATCH 15/20] update link for THisInExport bailout reason --- packages/transformers/js/core/src/utils.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/transformers/js/core/src/utils.rs b/packages/transformers/js/core/src/utils.rs index cb53bee5c60..c234aaedefb 100644 --- a/packages/transformers/js/core/src/utils.rs +++ b/packages/transformers/js/core/src/utils.rs @@ -358,7 +358,7 @@ impl BailoutReason { ), BailoutReason::ThisInExport => ( "Module contains `this` access of an exported value. This causes the module to be wrapped and tree-shaking to be disabled.", - "" + "https://parceljs.org/features/scope-hoisting/#avoiding-bail-outs" ), } } From a01708239200d7b32928f09586579ede51d87c1e Mon Sep 17 00:00:00 2001 From: achan3 Date: Fri, 6 Oct 2023 15:14:55 -0400 Subject: [PATCH 16/20] remove duplicate memberProp check --- packages/transformers/js/core/src/collect.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/transformers/js/core/src/collect.rs b/packages/transformers/js/core/src/collect.rs index fabbb0710dc..0f5eca7f30a 100644 --- a/packages/transformers/js/core/src/collect.rs +++ b/packages/transformers/js/core/src/collect.rs @@ -694,9 +694,7 @@ impl Visit for Collect { if self.in_module_this { handle_export!(); } else if !self.in_class { - if let MemberProp::Ident(prop) = &node.prop { - self.this_exprs.insert(id!(prop), node.clone()); - } + self.this_exprs.insert(id!(&node.prop), node.clone()); } return; } From 04c82d861915fd3bacd76b4d66fbbf314fffa142 Mon Sep 17 00:00:00 2001 From: achan3 Date: Fri, 6 Oct 2023 16:05:13 -0400 Subject: [PATCH 17/20] Revert "remove duplicate memberProp check" This reverts commit a01708239200d7b32928f09586579ede51d87c1e. --- packages/transformers/js/core/src/collect.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/transformers/js/core/src/collect.rs b/packages/transformers/js/core/src/collect.rs index 0f5eca7f30a..fabbb0710dc 100644 --- a/packages/transformers/js/core/src/collect.rs +++ b/packages/transformers/js/core/src/collect.rs @@ -694,7 +694,9 @@ impl Visit for Collect { if self.in_module_this { handle_export!(); } else if !self.in_class { - self.this_exprs.insert(id!(&node.prop), node.clone()); + if let MemberProp::Ident(prop) = &node.prop { + self.this_exprs.insert(id!(prop), node.clone()); + } } return; } From af247836bf51cfc7ea7f9257c1a6535c5e13dc1d Mon Sep 17 00:00:00 2001 From: Brian Tedder Date: Tue, 10 Oct 2023 14:46:09 -0500 Subject: [PATCH 18/20] store Ident, Span instead of entire node --- packages/transformers/js/core/src/collect.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/packages/transformers/js/core/src/collect.rs b/packages/transformers/js/core/src/collect.rs index fabbb0710dc..2122b66c435 100644 --- a/packages/transformers/js/core/src/collect.rs +++ b/packages/transformers/js/core/src/collect.rs @@ -58,7 +58,7 @@ pub struct Collect { pub should_wrap: bool, /// local variable binding -> descriptor pub imports: HashMap, - pub this_exprs: HashMap, + pub this_exprs: HashMap, /// exported name -> descriptor pub exports: HashMap, /// local variable binding -> exported name @@ -245,12 +245,10 @@ impl Visit for Collect { } self.in_module_this = false; - for (_key, node) in self.this_exprs.clone() { - if let MemberProp::Ident(prop) = &node.prop { - if self.exports.contains_key(&prop.sym) { - self.should_wrap = true; - self.add_bailout(node.span, BailoutReason::ThisInExport); - } + for (_key, (ident, span)) in self.this_exprs.clone() { + if self.exports.contains_key(&ident.sym) { + self.should_wrap = true; + self.add_bailout(span, BailoutReason::ThisInExport); } } @@ -695,7 +693,7 @@ impl Visit for Collect { handle_export!(); } else if !self.in_class { if let MemberProp::Ident(prop) = &node.prop { - self.this_exprs.insert(id!(prop), node.clone()); + self.this_exprs.insert(id!(prop), (prop.clone(), node.span)); } } return; From 751624c40aee5f3c5d36a0d70bd83c87b9260376 Mon Sep 17 00:00:00 2001 From: achan3 Date: Tue, 10 Oct 2023 16:57:27 -0400 Subject: [PATCH 19/20] move instead of clone self.this_exprs --- packages/transformers/js/core/src/collect.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/transformers/js/core/src/collect.rs b/packages/transformers/js/core/src/collect.rs index 2122b66c435..116f39fde1d 100644 --- a/packages/transformers/js/core/src/collect.rs +++ b/packages/transformers/js/core/src/collect.rs @@ -245,7 +245,9 @@ impl Visit for Collect { } self.in_module_this = false; - for (_key, (ident, span)) in self.this_exprs.clone() { + let this_exprs = std::mem::take(&mut self.this_exprs); + for (_key, (ident, span)) in this_exprs { + // for (_key, (ident, span)) in self.this_exprs.clone() { if self.exports.contains_key(&ident.sym) { self.should_wrap = true; self.add_bailout(span, BailoutReason::ThisInExport); From e33a45948a45d6e15fd3cdd94dd346a89f4f7c63 Mon Sep 17 00:00:00 2001 From: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com> Date: Wed, 11 Oct 2023 12:04:46 +0200 Subject: [PATCH 20/20] Cleanup --- packages/transformers/js/core/src/collect.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/transformers/js/core/src/collect.rs b/packages/transformers/js/core/src/collect.rs index 116f39fde1d..ee2ab4d64ed 100644 --- a/packages/transformers/js/core/src/collect.rs +++ b/packages/transformers/js/core/src/collect.rs @@ -245,9 +245,7 @@ impl Visit for Collect { } self.in_module_this = false; - let this_exprs = std::mem::take(&mut self.this_exprs); - for (_key, (ident, span)) in this_exprs { - // for (_key, (ident, span)) in self.this_exprs.clone() { + for (_key, (ident, span)) in std::mem::take(&mut self.this_exprs) { if self.exports.contains_key(&ident.sym) { self.should_wrap = true; self.add_bailout(span, BailoutReason::ThisInExport);