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: lazy load rollup/parseAst #15639

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

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Jan 18, 2024

Description

Lazy load rollup/parseAst, extracted from #15621

For #14833, if we want to continue to expose the sync parseAst from rollup, we need to do it by also exporting an initRollupParseAst function that should be called before it. This is breaking but I don't think there should be much usage for it yet. I can break the PR if needed but it would be good if we could merge it for Vite 5.1 and not have to wait for Vite 6

The init code is delayed now until it is really needed, and this means that for some projects, rollup/parseAst won't be even loaded during dev.

This is a breaking change, but we didn't document this.parse, and the exported parseAst function. Maybe it is ok still for Vite 5.1 given that vite ecosystem CI is happy with the change. My take is that we should only expose this.parseAsync and parseAstAsync instead in Vite. I don't think we need the sync versions. cc @sheremet-va


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

stackblitz bot commented Jan 18, 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 18, 2024
@sheremet-va
Copy link
Member

My take is that we should only expose this.parseAsync and parseAstAsync instead in Vite. I don't think we need the sync versions

Vitest uses this.parse for its mocking implementation and it's sync. Vitest also uses parseAstAsync to parse files for its type-checking feature. We don't use parseAst directly though.

sheremet-va
sheremet-va previously approved these changes Jan 18, 2024
@patak-dev
Copy link
Member Author

@sheremet-va if Vitest uses this.parse, then I think we would have to move the initRollupParseAst back to buildStart or server init. Or Vitest will need to call it before using the function like this PR is doing. I don't know what is best here, it feels very wasteful to always load rollup/parseAst if it is only going to be used in some projects during dev.

@sheremet-va
Copy link
Member

sheremet-va commented Jan 18, 2024

@sheremet-va if Vitest uses this.parse, then I think we would have to move the initRollupParseAst back to buildStart or server init. Or Vitest will need to call it before using the function like this PR is doing. I don't know what is best here, it feels very wasteful to always load rollup/parseAst if it is only going to be used in some projects during dev.

this.parse API existed before parseAst was introduced (so before rollup 4). I don't think we can just stop supporting it, it might be used by projects other than Vitest.

@sheremet-va
Copy link
Member

I am fine with calling initRollupParseAst on Vitest side, it would just mean that Vitest 1.0 doesn't support Vite 5.1

@sheremet-va
Copy link
Member

this.parse API existed before parseAst was introduced (so before rollup 4).

And it existed because it's a rollup API so it doesn't need documentation.

@patak-dev
Copy link
Member Author

this.parse API existed before parseAst was introduced (so before rollup 4).

And it existed because it's a rollup API so it doesn't need documentation.

I wrongly thought it was added in Vite 5, I got confused with it changing from acorn to use rollup/parseAst there.

I moved the init to be awaited in buildStart. So the only breaking change should be when calling the re-exported sync parseAst from vite before buildStart is called, and this function wasn't documented so we may be fine to merge this in 5.1 then.

@patak-dev
Copy link
Member Author

/ecosystem-ci run vitest

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on ad32d57: Open

suite result latest scheduled
vitest success success

@sapphi-red
Copy link
Member

Does importing rollup/parseAst take time? I wonder if dynamically importing rollup and statically importing rollup/parseAst has a good balance.

@patak-dev
Copy link
Member Author

Does importing rollup/parseAst take time? I wonder if dynamically importing rollup and statically importing rollup/parseAst has a good balance.

This is an option too. I'll try to get some numbers after we merge #15621 and rollup is no longer statically imported. If we do this, maybe it still makes sense to remove the static parseAst re-export so the ecosystem doesn't start to use it (given that it wasn't documented)

@patak-dev patak-dev marked this pull request as draft January 22, 2024 15:27
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