-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Remove this
exports tracking for files with module syntax
#9330
Changes from 3 commits
3843469
993fc0a
7103543
4ab7e55
dddfd19
508b74d
c9647a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated this to closer reflect the parsing we do in |
||
Ok(program) => swc_core::common::GLOBALS.set(&Globals::new(), || { | ||
swc_core::ecma::transforms::base::helpers::HELPERS.set( | ||
&swc_core::ecma::transforms::base::helpers::Helpers::new(false), | ||
|| { | ||
let is_module = program.is_module(); | ||
let module = match program { | ||
Program::Module(module) => module, | ||
Program::Script(script) => Module { | ||
span: script.span, | ||
shebang: None, | ||
body: script.body.into_iter().map(ModuleItem::Stmt).collect(), | ||
}, | ||
}; | ||
|
||
let unresolved_mark = Mark::fresh(Mark::root()); | ||
let global_mark = Mark::fresh(Mark::root()); | ||
let module = module.fold_with(&mut resolver(unresolved_mark, global_mark, false)); | ||
|
@@ -1155,6 +1165,7 @@ mod tests { | |
Mark::fresh(Mark::root()), | ||
global_mark, | ||
true, | ||
is_module, | ||
); | ||
module.visit_with(&mut collect); | ||
|
||
|
@@ -1444,6 +1455,7 @@ mod tests { | |
// We want to avoid marking these modules as CJS | ||
let (collect, _code, _hoist) = parse( | ||
r#" | ||
import 'something'; | ||
var __classPrivateFieldSet = (this && this.__classPrivateFieldSet) || function () {} | ||
"#, | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this do exactly? What is the definition of a "module" here? Anything with an import/export? We also compute this within There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Looks like yes https://github.com/swc-project/swc/blob/a18ffc107f779d2fb576d610af65315e460efbd1/crates/swc_ecma_parser/src/parser/mod.rs#L162 |
||
// If it's a script, convert into module. This needs to happen after | ||
// the resolver (which behaves differently for non-/strict mode). | ||
let module = match module { | ||
|
@@ -342,6 +343,7 @@ pub fn transform(config: Config) -> Result<TransformResult, std::io::Error> { | |
global_mark, | ||
&config.project_root, | ||
&mut fs_deps, | ||
is_module | ||
), | ||
should_inline_fs | ||
), | ||
|
@@ -448,6 +450,7 @@ pub fn transform(config: Config) -> Result<TransformResult, std::io::Error> { | |
ignore_mark, | ||
global_mark, | ||
config.trace_bailouts, | ||
is_module, | ||
); | ||
module.visit_with(&mut collect); | ||
if let Some(bailouts) = &collect.bailouts { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might still have to track this_exprs in ESM (the second case below)? For example:
Looks like we only have a CJS test for that (in hoist.rs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you do
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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.