Remove this exports tracking for files with module syntax
#9330
Conversation
Benchmark ResultsKitchen Sink ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. React HackerNews ✅
Timings
Cold Bundles
Cached Bundles
AtlasKit Editor ✅
Timings
Cold Bundles
Cached Bundles
Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
| @@ -252,6 +252,7 @@ pub fn transform(config: Config) -> Result<TransformResult, std::io::Error> { | |||
| ), | |||
| )); | |||
|
|
|||
| let is_module = module.is_module(); | |||
There was a problem hiding this comment.
What does this do exactly? What is the definition of a "module" here? Anything with an import/export? We also compute this within collect itself already.
There was a problem hiding this comment.
Anything with an import/export
Looks like yes https://github.com/swc-project/swc/blob/a18ffc107f779d2fb576d610af65315e460efbd1/crates/swc_ecma_parser/src/parser/mod.rs#L162
|
Is the problem that the member expression is still being tracked as an export? REPL I guess that would happen here: So you might need the same binary expression check in |
Yeah I believe that's the case, but we don't track top-level
I agree it'd be better to only track assignments but that's a much larger task. Do you have any blocking concerns for this fix? Without it we're breaking CJS files that have the TSC polyfills. |
|
With this change, do we even need the previous one? We could just disable |
|
@devongovett 🤔 Good point, I hadn't actually considered that. The only thing I can think of is potentially breaking I'll rework to disable |
| @@ -1140,11 +1140,21 @@ mod tests { | |||
| ); | |||
|
|
|||
| let mut parser = Parser::new_from(lexer); | |||
| match parser.parse_module() { | |||
| Ok(module) => swc_core::common::GLOBALS.set(&Globals::new(), || { | |||
| match parser.parse_program() { | |||
There was a problem hiding this comment.
I updated this to closer reflect the parsing we do in lib.rs
mattcompiles
changed the title
this exports tracking for files with module syntax
|
@mischnic @devongovett I updated the PR to instead remove |
| @@ -689,6 +692,11 @@ impl Visit for Collect { | |||
| return; | |||
| } | |||
| Expr::This(_this) => { | |||
| if self.is_module { | |||
| // Don't track this bindings if ESM syntax is present | |||
There was a problem hiding this comment.
I think we might still have to track this_exprs in ESM (the second case below)? For example:
export function a() {
return this.b();
}
export function b() {
return 2;
}Looks like we only have a CJS test for that (in hoist.rs).
There was a problem hiding this comment.
This isn't valid code though as this is undefined for ESM files. Is there actual scenarios expecting this to work?
There was a problem hiding this comment.
if you do
import * as ns from './above_code';
ns.a();the this will be the namespace. Niklas brought this example up in the issue that the recent PR fixed. #7866 (comment)
There was a problem hiding this comment.
@devongovett I feel like this is an accidental implementation detail of ESM on top of existing JS. No one would do it in practice as it means you're library could only be used via namespace imports. Not sure we should go out of our way to support this.
There was a problem hiding this comment.
I only brought it up because I think this PR literally just fixed it! #9291 Does it work if you only make the first case conditional on ESM instead of the whole thing?
There was a problem hiding this comment.
@devongovett I'm a bit confused on this. That fix was for CJS files which still works. The ESM scenario doesn't work regardless of this change. If I add a test with the code Niklas mentioned in that PR it fails with and without my changes.
Does it work if you only make the first case conditional on ESM instead of the whole thing?
It does work in this case. I'll update the code to this but I'm not sure tracking this in ESM files is really doing anything.
Co-authored-by: Devon Govett <[email protected]>
↪️ Pull Request
The TSC polyfill check was breaking CJS files as we need to track
thisreferences for exports. This PR makes it so we disablethisexport tracking when any module syntax is present.✔️ PR Todo