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

perf: use magic-string hires boundary for sourcemaps #13971

Merged
merged 1 commit into from
Jul 29, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jul 28, 2023

Description

Generate sourcemap segments per word boundary instead of per character. Rich-Harris/magic-string#255

I didn't test this a lot but I think everything should stay the same. We're using magic-string to manipulate JS and CSS, which are code that are semantically separated by word boundaries.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@stackblitz
Copy link

stackblitz bot commented Jul 28, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev
Copy link
Member

Awesome work! Tempted to release this one in a patch, but it is probably better to merge it in the Vite 5 beta.

@bmeurer maybe you could help us check this one out?

@patak-dev patak-dev added p4-important Violate documented behavior or significantly improves performance (priority) feat: sourcemap Sourcemap support labels Jul 28, 2023
@patak-dev patak-dev added this to the 5.0 milestone Jul 28, 2023
@bluwy
Copy link
Member Author

bluwy commented Jul 28, 2023

I ran the benchmark at https://github.com/vitejs/vite-benchmark/actions/runs/5689588084 and it doesn't seem to affect performance, so maybe it only helps a bit or the benchmark fixtures aren't generating many sourcemaps.

@sapphi-red
Copy link
Member

Awesome that you implemented this! @bmeurer suggested a similar way a bit ago (#13514 (comment)) to me.

I ran the benchmark at https://github.com/vitejs/vite-benchmark/actions/runs/5689588084 and it doesn't seem to affect performance, so maybe it only helps a bit or the benchmark fixtures aren't generating many sourcemaps.

I guess that's because vitejs/vite-benchmark doesn't run benchmarks for build.

@bluwy
Copy link
Member Author

bluwy commented Jul 29, 2023

Awesome work! Tempted to release this one in a patch, but it is probably better to merge it in the Vite 5 beta.

I think it should be rather safe to merge this one. It shouldn't degrade the sourcemap when using it.

@patak-dev
Copy link
Member

Ok, let's try it out

@patak-dev patak-dev merged commit b9a8d65 into main Jul 29, 2023
@patak-dev patak-dev deleted the magic-string-boundary branch July 29, 2023 13:56
@patak-dev patak-dev removed this from the 5.0 milestone Jul 29, 2023
@bmeurer
Copy link
Contributor

bmeurer commented Jul 31, 2023

Nice one, thanks!

This might be interesting to bring up with the TC39-TG4 in the context of source map standardization. cc @jaro-sevcik @littledan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: sourcemap Sourcemap support p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants