feat(minifier): improve peephole-replace-known-methods
#6490
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
CodSpeed Performance ReportMerging #6490 will not alter performanceComparing Summary
|
shulaoda
force-pushed
the
feat/improve-peephole-replace-known-methods
branch
from
b810491 to
5772d79
Compare
crates/oxc_minifier/src/ast_passes/peephole_replace_known_methods.rs
Outdated
Show resolved
Hide resolved
shulaoda
force-pushed
the
feat/improve-peephole-replace-known-methods
branch
from
a102a0d to
7dbd25c
Compare
crates/oxc_minifier/src/ast_passes/peephole_replace_known_methods.rs
Outdated
Show resolved
Hide resolved
crates/oxc_minifier/src/ast_passes/peephole_replace_known_methods.rs
Outdated
Show resolved
Hide resolved
| fold_same("x = `abcdef`.indexOf('b')"); | ||
| fold("x = `abcdef`.indexOf('b')", "x = 1;"); |
There was a problem hiding this comment.
Our test infra are ported from Google Closure Compiler, where it does not fold the template strings, and related code here: https://github.com/google/closure-compiler/blob/master/test/com/google/javascript/jscomp/PeepholeReplaceKnownMethodsTest.java#L114.
@Boshen any suggestions?
There was a problem hiding this comment.
@shulaoda The template string contains some expressions, but some of them can't be handled. There are already methods in place to replace template strings with normal strings when possible, and the minification process iterates multiple times rather than being a single pass. So, I believe we can focus on the string itself and not worry about template strings here. Everything else looks fantastic—thanks for your contribution!
There was a problem hiding this comment.
So don't worry these tests here - after several times, from the template string, to the normal string, the minifier can handle this scenario :-)
There was a problem hiding this comment.
You are correct, I saw that the closure-compiler did recognize and convert the TemplateLiteral.
shulaoda
force-pushed
the
feat/improve-peephole-replace-known-methods
branch
2 times, most recently
from
7c80f97 to
3bbf0f9
Compare
| #[test] | ||
| fn test_get_string_literal() { |
There was a problem hiding this comment.
I don't know how to implement the test template and where to place it.
There was a problem hiding this comment.
I have implemented a simple test case and temporarily placed it here.
There was a problem hiding this comment.
I would recommend you to put it to src/node_util/mod.rs instead.
There was a problem hiding this comment.
To be honest, I'm not sure if it's still necessary to exist. Because our TemplateLiteral seems to be able to be compressed into StringLiteral. I don't know if there are any specific scenarios that require this method.
shulaoda
force-pushed
the
feat/improve-peephole-replace-known-methods
branch
from
3bbf0f9 to
fba1884
Compare
shulaoda
changed the title
peephole-replace-known-methods
Support code similar to below: