Skip to content

feat(minifier): complete MangleIf#8810

Merged
Boshen merged 1 commit intomainfrom
01-31-feat_minifier_complete_mangleif_
Feb 2, 2025
Merged

feat(minifier): complete MangleIf#8810
Boshen merged 1 commit intomainfrom
01-31-feat_minifier_complete_mangleif_

Conversation

@Boshen
Copy link
Member

@Boshen Boshen commented Jan 31, 2025

No description provided.

@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 31, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@github-actions github-actions bot added A-minifier Area - Minifier C-enhancement Category - New feature or request labels Jan 31, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 31, 2025

CodSpeed Performance Report

Merging #8810 will not alter performance

Comparing 01-31-feat_minifier_complete_mangleif_ (9429b26) with main (d553318)

Summary

✅ 33 untouched benchmarks

@Boshen Boshen force-pushed the 01-31-feat_minifier_complete_mangleif_ branch 4 times, most recently from bcbbf8e to 6f9a596 Compare January 31, 2025 15:59
@github-actions github-actions bot added the A-ast Area - AST label Jan 31, 2025
@Boshen Boshen force-pushed the 01-31-feat_minifier_complete_mangleif_ branch from 6f9a596 to 13114a2 Compare February 1, 2025 14:27
@Boshen Boshen requested a review from sapphi-red February 1, 2025 14:28
@Boshen Boshen marked this pull request as ready for review February 1, 2025 14:28
@Boshen
Copy link
Member Author

Boshen commented Feb 1, 2025

There's probably a bunch of missed minifications for if (foo) statement vs if (foo) { statement }, but this is a good starting point.

@Boshen Boshen marked this pull request as draft February 1, 2025 14:37
@Boshen
Copy link
Member Author

Boshen commented Feb 1, 2025

Failed many idempotency tests in monitor-oxc :-/

@sapphi-red
Copy link
Member

Maybe because self.mark_current_function_as_changed is only called in some code paths?

@Boshen
Copy link
Member Author

Boshen commented Feb 1, 2025

Maybe because self.mark_current_function_as_changed is only called in some code paths?

Related to ambiguous if else (dangling else)

function K(G, X, B) {
  var U, Z = iK[G];
  if (U = typeof Z == "string" ? Z : X === 1 ? Z.one : Z.other.replace("{{count}}", X.toString()), B != null && B.addSuffix)
    if (B.comparison && B.comparison > 0) return "in " + U;
  else return U + " ago";
  return U;
}

and leaving behind 1 in if (true) instead of if(!0)

if (true){
    var __func = function(){return HERE;};
} else {
    var __func = function (){return THERE;};
};

@Boshen
Copy link
Member Author

Boshen commented Feb 1, 2025

@sapphi-red I had a long day today so I'll stop here, feel free to continue on this PR if you want.

We need to rework try_fold_if in remove_dead_code.rs (remove duplicated logic), and implement https://github.com/evanw/esbuild/blob/745abd9f0c06f73ca40fbe198546a9bc36c23b81/internal/js_parser/js_parser.go#L9896-L9902

I haven't fully understood how esbuild works around the dangling else problem in the minifier, but just in case you missed ... it adds the block in codegen

stmt if wrap_to_avoid_ambiguous_else(stmt) => {

and I added wrap_to_avoid_ambiguous_else to work around the missed minification.

@sapphi-red
Copy link
Member

I'll be stopping in a hour or so as well, so probably only going to add some comments in this PR.

@Boshen Boshen force-pushed the 01-31-feat_minifier_complete_mangleif_ branch from c00f3f7 to 6bac151 Compare February 2, 2025 01:31
@sapphi-red
Copy link
Member

sapphi-red commented Feb 2, 2025

Pushed a commit that fixes monitor-oxc expect for the following one that is also failing in main branch

node_modules/.pnpm\ts-morph@25.0.0\node_modules\ts-morph\dist\ts-morph.js
-               if (currentChar === "\n" || currentChar !== " " && currentChar !== "    ") return pos + 1;
+               if (currentChar === "\n") return pos + 1;
+               if (currentChar !== " " && currentChar !== "    ") return pos + 1;

@sapphi-red sapphi-red force-pushed the 01-31-feat_minifier_complete_mangleif_ branch from dae267e to 29dd860 Compare February 2, 2025 07:57
Copy link
Member


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Boshen Boshen force-pushed the 01-31-feat_minifier_complete_mangleif_ branch 2 times, most recently from dabe3e5 to 3a8440e Compare February 2, 2025 08:58
@Boshen Boshen marked this pull request as ready for review February 2, 2025 08:58
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Feb 2, 2025
Copy link
Member Author

Boshen commented Feb 2, 2025

Merge activity

  • Feb 2, 4:05 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Feb 2, 4:05 AM EST: A user added this pull request to the Graphite merge queue.

graphite-app bot pushed a commit that referenced this pull request Feb 2, 2025
@graphite-app graphite-app bot force-pushed the 01-31-feat_minifier_complete_mangleif_ branch from 3a8440e to 4a7a965 Compare February 2, 2025 09:05
graphite-app bot pushed a commit that referenced this pull request Feb 2, 2025
@graphite-app graphite-app bot force-pushed the 01-31-feat_minifier_complete_mangleif_ branch from 4a7a965 to 422c84c Compare February 2, 2025 09:09
graphite-app bot pushed a commit that referenced this pull request Feb 2, 2025
@graphite-app graphite-app bot force-pushed the 01-31-feat_minifier_complete_mangleif_ branch from 422c84c to f1f743d Compare February 2, 2025 09:13
@graphite-app graphite-app bot force-pushed the 01-31-feat_minifier_complete_mangleif_ branch from f1f743d to 9429b26 Compare February 2, 2025 09:20
@Boshen Boshen merged commit d553318 into main Feb 2, 2025
23 checks passed
@Boshen Boshen deleted the 01-31-feat_minifier_complete_mangleif_ branch February 2, 2025 09:21
@oxc-bot oxc-bot mentioned this pull request Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-merge Merge with Graphite Merge Queue A-ast Area - AST A-minifier Area - Minifier C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants