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: escape replacements in clientInjections #12486

Merged

Conversation

d-fischer
Copy link
Contributor

Description

This fixes a bug where special regex replacement characters are wrongly replaced. This can easily break a build. For example, this configuration always fails to compile:

import { defineConfig } from "vite";

export default defineConfig({
  define: {
    VAR_FOO: '"$`"'
  }
});

I also added the same fix to all other replacements in the same block of code, just to be extra sure.


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 Mar 19, 2023

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

@patak-dev
Copy link
Member

Thanks for the PR @d-fischer. We are discussing with @bluwy about performance on internal plugins, so I pushed a commit here to take the opportunity to gain some ms after your changes.

@bluwy testing with https://github.com/antfu/icones and DEBUG="vite:plugin-transform" pnpm dev, I'm getting ~100ms for the 3 times vite:client-inject is called on my machine. For both main and after this PR without my modifications. If there is a regression in perf because of these changes it would be hard to measure.

After vitejs/vite@27bf6dd (#12486), the transform can be a sync function, and it is now reporting ~0.3ms for the 3 calls. The buildStart got a new async function in its way, but I measured the time to run the internal logic and it is around consistently ~5ms. Maybe it is easier to optimize the new injectConfigValues. I think having one less async transform function is good in general as this hook will be called for every module. I think we should also extract the regex for the other branch. I'll do that in another PR.

@patak-dev
Copy link
Member

Ah, nice one @d-fischer, I missed this one vitejs/vite@7e46ca2 (#12486) ❤️

@patak-dev patak-dev added p4-important Violate documented behavior or significantly improves performance (priority) performance Performance related enhancement labels Mar 20, 2023
@patak-dev patak-dev added this to the 4.3 milestone Mar 20, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

NIce! I might've wrongly remembered the speed for client-inject before, but I can confirm this PR helps a lot too. It would be nice if buildStart doesn't block the server startup too, but given it's only 2-4ms , I'm fine with it for now too.

@patak-dev patak-dev merged commit 3765067 into vitejs:main Mar 21, 2023
@d-fischer d-fischer deleted the escape-replacement-in-client-injections branch March 21, 2023 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority) performance Performance related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants