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: stricter dynamicImportVars transform filter #15578

Closed
wants to merge 1 commit into from

Conversation

patak-dev
Copy link
Member

Description

The @react-refresh virtual module has:

const __hmr_import = (module)=>import(/* @vite-ignore */
__vite__injectQuery(module, 'import'));

This is the only dynamic import and should be ignored, but the fast short-circuit regex in the dynamic import vars plugin fails to discard this case. This PR makes the guard stricter so @react-refresh and other similar modules won't need to be parsed.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

stackblitz bot commented Jan 11, 2024

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

@patak-dev patak-dev added the performance Performance related enhancement label Jan 11, 2024
@bluwy
Copy link
Member

bluwy commented Jan 11, 2024

Maybe it's worth benchmarking how long it'll take the regex to run with this PR 🤔 In the past I've found that longer regexes slows down it's matching, so this PR could create a fast-path for @react-refresh, but slows down matching for all other modules.

@patak-dev
Copy link
Member Author

To be honest, I don't know how to properly measure the impact. Checking with perf.link, I see it is a bit slower but it is quite noisy when I try to compared against another base.

@bluwy
Copy link
Member

bluwy commented Jan 22, 2024

I think according to the perf link, it might not be worth optimizing this case 🤔 It does show an overall slow down especially that the pattern won't exist in most files.

@patak-dev
Copy link
Member Author

Ok, let's close this one. @ArnaudBarre you could add @react-refresh to build.dynamicImportVarsOptions and it seems that it is also going to get then filtered during dev... I don't think we should support this config option as is though 🤔
I think somehow it will be nice that @react-refresh isn't parsed by acorn every time.

@patak-dev patak-dev closed this Jan 22, 2024
@ArnaudBarre
Copy link
Member

This import was moved to the shared utils for perf reason already in vitejs/vite-plugin-react#143
So this should be parsed only once, I think this is fine

@patak-dev patak-dev deleted the perf/dynamic-import-var-guard branch March 22, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants