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.

Sign up for GitHub

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

Jump to bottom

feat: replace top-level this in esm #4497

Merged
merged 1 commit into from
Nov 2, 2023
Merged

feat: replace top-level this in esm #4497

merged 1 commit into from
Nov 2, 2023

Conversation

Austaras
Copy link
Contributor

@Austaras Austaras commented Nov 1, 2023

Summary

Closes #4421

Test Plan

Require Documentation?

  • No
  • Yes, the corresponding rspack-website PR is __

@Austaras Austaras requested review from a team, LingyuCoder and ahabhgk November 1, 2023 05:10
@CLAassistant
Copy link

CLAassistant commented Nov 1, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the release: feature release: feature related release(mr only) label Nov 1, 2023
@ahabhgk
Copy link
Contributor

ahabhgk commented Nov 1, 2023 •
edited
Loading

I think now webpack-test/cases/parsing/es2020 can pass, could you enable it, just need to make test.filter.js return true, and run pnpm run test -- -t "parsing es2020" under webpack-test folder

@IWANABETHATGUY
Copy link
Contributor

I think now webpack-test/cases/parsing/es2020 can pass, could you enable it, just need to make test.filter.js return true, and run pnpm run test -- -t "parsing es2020" under webpack-test folder

string replacing-based codegen required ast to be untouched (some plugins needs span info), if we add an extra transforming pass, it may affect the optimization result. Can we move the transforming work to the loader side?

@IWANABETHATGUY
Copy link
Contributor

Or just like the webpack, use aConstDependency instead.

@Austaras
Copy link
Contributor Author

Austaras commented Nov 1, 2023

Turns out the origin impl doesn't work at all. Two separate AST is used for codegen and dep analysis, and modify the latter one doesn't have any effect.

@Austaras
Copy link
Contributor Author

Austaras commented Nov 1, 2023

I think now webpack-test/cases/parsing/es2020 can pass, could you enable it, just need to make test.filter.js return true, and run pnpm run test -- -t "parsing es2020" under webpack-test folder

string replacing-based codegen required ast to be untouched (some plugins needs span info), if we add an extra transforming pass, it may affect the optimization result. Can we move the transforming work to the loader side?

Seems like most passes in transform_before_pass belong in the loader, but for this PR I believe it's better not to change this logic.

ahabhgk
ahabhgk reviewed Nov 1, 2023
View reviewed changes
Copy link
Contributor

@ahabhgk ahabhgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better if you can implement it by string-replacement way (use ConstDependency to replace this to undefined), this is pretty similar with ReplaceNestWebpackRequireVisitor, so you can reference it at

rspack/crates/rspack_plugin_javascript/src/visitors/dependency/compatibility_scanner.rs

Lines 52 to 75 in 3c0c6ca

pub struct ReplaceNestWebpackRequireVisitor<'a> {
unresolved_ctxt: &'a SyntaxContext,
name_map: &'a FxHashMap<SyntaxContext, u8>,
presentational_dependencies: &'a mut Vec<Box<dyn DependencyTemplate>>,
}
impl Visit for ReplaceNestWebpackRequireVisitor<'_> {
noop_visit_type!();
fn visit_ident(&mut self, ident: &Ident) {
if &ident.sym == "__webpack_require__" && ident.span.ctxt != *self.unresolved_ctxt {
if let Some(count) = self.name_map.get(&ident.span.ctxt) {
self
.presentational_dependencies
.push(Box::new(ConstDependency::new(
ident.span.real_lo(),
ident.span.real_hi(),
format!("__nested_webpack_require_{count}__").into(),
None,
)));
}
}
}
}

crates/rspack_plugin_javascript/src/visitors/harmony_top_level_this.rs Outdated Show resolved Hide resolved
crates/rspack_plugin_javascript/src/visitors/mod.rs Outdated Show resolved Hide resolved
@Austaras
Copy link
Contributor Author

Austaras commented Nov 2, 2023

Done. But what's the difference between AST plugin and text plugin? What's the necessity of the latter?

@ahabhgk
Copy link
Contributor

ahabhgk commented Nov 2, 2023 •
edited
Loading

We used ast-based replacement for all our previous implementations, but we found it challenging to implement caching and persistent caching using ast compared to string replacement. Therefore, we switched to the implementation of string replacement, which is currently in an intermediate stage. We currently have both string replacement and ast replacement, as there are still some blockers for the remaining ast replacement. In the long term, we will replace all of it with string replacement.

@ahabhgk
Copy link
Contributor

ahabhgk commented Nov 2, 2023

LGTM, can you make the CI passes

@Austaras
Copy link
Contributor Author

Austaras commented Nov 2, 2023 •
edited
Loading

Interesting. Maybe it would be doable after SWC finishes it's plugin story which involves of handling of serializing with rykv

@ahabhgk
Copy link
Contributor

ahabhgk commented Nov 2, 2023

Thanks!

@ahabhgk ahabhgk enabled auto-merge November 2, 2023 04:35
@ahabhgk ahabhgk added this pull request to the merge queue Nov 2, 2023
Merged via the queue into web-infra-dev:main with commit 11b101b Nov 2, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahabhgk ahabhgk ahabhgk approved these changes

@LingyuCoder LingyuCoder Awaiting requested review from LingyuCoder

@IWANABETHATGUY IWANABETHATGUY Awaiting requested review from IWANABETHATGUY

@ScriptedAlchemy ScriptedAlchemy Awaiting requested review from ScriptedAlchemy

Assignees
No one assigned
Labels
release: feature release: feature related release(mr only)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

should replace top-level this to undefined in esm
4 participants
@Austaras @CLAassistant @ahabhgk @IWANABETHATGUY