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.

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

fix(mangler): keep exported symbols for top_level: true #7927

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sapphi-red
Copy link
Contributor

@sapphi-red sapphi-red commented Dec 16, 2024

I'm not going to work on a fix for a while so feel free to fix it if anyone wants to work on it.

Exported symbols are now not mangled.

Copy link

graphite-app bot commented Dec 16, 2024

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-test Category - Testing. Code is missing test cases, or a PR is adding them labels Dec 16, 2024
Copy link

codspeed-hq bot commented Dec 16, 2024

CodSpeed Performance Report

Merging #7927 will not alter performance

Comparing sapphi-red:test/mangle-top-level-vars (fe885dc) with main (547c102)

Summary

✅ 29 untouched benchmarks

@camc314
Copy link
Contributor

camc314 commented Dec 16, 2024

@ Boshen, what's the right way to fix this one?

we probably need access to ModuleRecord in this mangler to check what's exported/not right?

@sapphi-red sapphi-red force-pushed the test/mangle-top-level-vars branch from 41aa4a0 to c2a99eb Compare December 22, 2024 07:58
@sapphi-red sapphi-red changed the title test(mangler): failing test for exported top level variables fix(mangler): keep exported symbols for top_level: true Dec 22, 2024
@github-actions github-actions bot added the C-bug Category - Bug label Dec 22, 2024
@sapphi-red
Copy link
Contributor Author

I made the exported symbols to be not mangled. I made a function that iterates over the top-level statements to collect the exported symbols.

@sapphi-red sapphi-red marked this pull request as ready for review December 22, 2024 08:17
@Boshen Boshen self-assigned this Dec 22, 2024
Copy link
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

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

Terser's toplevel option seems different to our top_level, is there an existing option in another minifier? Or perhaps we should never mangle top level symbols, as it doesn't make sense.

@Boshen
Copy link
Member

Boshen commented Dec 23, 2024

@ Boshen, what's the right way to fix this one?

we probably need access to ModuleRecord in this mangler to check what's exported/not right?

Code may have been changed a lot after minfication, I think it's best to recollect them. Probably need something better to get some code reuse.

})
.map(|id| (id.name.to_compact_str(), id.symbol_id()))
.collect()
}
Copy link
Member

Choose a reason for hiding this comment

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

The best approach is to have a symbol flag for the exports 🤔 but there aren't any...

@sapphi-red
Copy link
Contributor Author

Terser's toplevel option seems different to our top_level

Is it different?
For input:

export const foo = ''
const bar = ''
export { bar }

terser outputs

/*
option:
{
  module: false,
  mangle: { toplevel: true }
}
*/
export const foo="";const o="";export{o as bar};

/*
option:
{
  module: false,
  mangle: { toplevel: false }
}
*/
export const foo="";const bar="";export{bar};

on https://try.terser.org/.

Or perhaps we should never mangle top level symbols, as it doesn't make sense.

Both:

  • changing the behavior on whether the input contains ESM syntaxes like esbuild does
  • having an option to specify the input/output format like SWC does

works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-minifier Area - Minifier C-bug Category - Bug C-test Category - Testing. Code is missing test cases, or a PR is adding them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants