feat(hmr): experimental.hmrPartialAccept #7324
Conversation
Niputi
added
enhancement
|
Looks like you need to add @types/parse-static-imports |
@mkilpatrick Unfortunately, the types don't exist apparently... Anyway, I don't believe that I would have preferred to use parse-imports actually, because it also extracts names from dynamic imports, which would be better for HMR too (when possible). But I met some difficulties to get Vite to build with it... In the end, Vite's maintainers might prefer to use something else, or extract and internalize the relevant bits, I don't know. For now, I'll wait for the discussion about the feature itself to happen before digging too much into the technical intricacies. |
|
Ah ok, sounds good. My use case is interesting because the second exported function is irrelevant and will never be imported by another module. It's solely used for a production build purpose to do some external things via a separate custom plugin. Edit: I was hoping that there could be some sort of flag to tell Vite "only care about the default export" or something like that. |
|
@rixo we discussed the proposal today with the team, and we think that both the use case and the proposed API are good. As you said, the main issue is how this could affect performance, both in time and the space complexity with the new data in the module graph. The library you used for the POC, as you described is good to get something working but we should probably explore other options. To be able to move forward, we could get the PR to a state where the performance impact could be measured. If the impact isn't that big, the ideal would be to avoid adding a new option to enable/disable this feature. But if not, we should make this optional, and allow things to continue working in the way they do now for people that don't care about the extra HMR granularity. Vite 2.9 will probably be released next week. And we are going to start the beta for Vite 3.0 in a month or so. Maybe this is a good feature to work for the next major. |
patak-dev
added
p2-nice-to-have
|
@patak-dev That's very good news!
Actually, AFAICT the PR already works, even if there's obviously some room for optimization and some enhancements that can be discussed. Do you have any existing procedure or some suggestions on how we could proceed to measure the performance impact? |
| const [parsed] = parseStaticImports(exp) | ||
| if (!parsed) { | ||
| return | ||
| } | ||
| if (parsed.sideEffectOnly) { | ||
| return | ||
| } | ||
| if (parsed.starImport) { | ||
| bindings.add('*') | ||
| } | ||
| if (parsed.defaultImport) { | ||
| bindings.add('default') | ||
| } | ||
| if (parsed.namedImports) { | ||
| for (const { name } of parsed.namedImports) { | ||
| bindings.add(name) | ||
| } | ||
| } |
There was a problem hiding this comment.
I wonder if Vite can/should roll its own parser instead of relying on parse-static-imports. @poyoho recently made refactors to emptying comments in strings easily (using emptyString()), and the import spec could be simple to parse manually (i hope). That way we could reduce one dep from bundling, and we could guarantee performance on this as es-module-lexer is part of the lexing process.
There was a problem hiding this comment.
We could also check out @pi0 and @danielroe mlly, which includes fast regex-based utils to extract imports: https://github.com/unjs/mlly#importexport-analyzes
There was a problem hiding this comment.
I have replaced parse-static-imports with mlly. The function we need looks like it would be easy enough to extract and inline, if desired. Would that be a good idea?
I have also been looking at parse-imports, which would probably do the job too. It's using a lexing approach, that intuitively seems like it could be faster than regexes, but it is implemented in JS so it might not be the case at all, in practice. Also, it would apparently be more cumbersome to extract just the part we need.
mlly seems to strike a better balance for our needs.
I'm not too sure why this change has caused some of the test setups to fail. Any idea?
There was a problem hiding this comment.
I wonder if Vite can/should roll its own parser
To be honest, I'm interested in it, but this will be a breakthrough change. Not only import expression but also lex assets expression, worker expression and any more.
The main advantage is to process these matches in the same state machine, and you can directly ignore comments and strings. There is no need to save the original and clean state.
The disadvantages are also obvious. The readability is not as strong as regexp, and the need for a large area of refactors may cause many regression problems.
|
@patak-dev Any idea what would be the next step to help move this forward? |
|
CI errors looks like a glitch, if they remain after you rebase, we'll take a closer look. I think the PR is already in a state where performance can be measured. It doesn't need to be a full study but at least have some large SvelteKit, Nuxt v3, or Hydrogen apps and see how loading time and HMR are affected. And if things look good, we can merge it so the ecosystem can play with it during the beta. We could delay adding an option to enable this feature so we get more testing, and see if it is really needed. |
|
@rixo since we're close to releasing Vite v3, we talked with the team to allow you to start experimenting with the new API. I think we would not be able to move the feature forward until we include it in a release and others are able to test it. Would it be possible for you to merge main and add a new |
|
Sure, I'm on it. Thanks a lot :) |
|
I have merged the latest Is there anything more to be done at this point? Should we mention the experimental feature in the docs already? I'm awaiting further instructions, if needed. |
|
@rixo amazing work here, thanks for pushing it to the finish line on short notice. I don't think we should document it yet, it would be good if we first validate the API. I imagine you are going to try to get this working in svelte, and that already is going to be a good proof for the design. If you would like, I think you could document the current approach and release it as a blog post clearly stating the experimental nature of the feature, so we can share it and hopefully get others involved. And we can also reach out to some maintainers directly. Let's get this one in beta.0 |
patak-dev
changed the title
|
We're looking for feedback about stabilizing this feature: |
Description
For now, this PR provides an example implementation of HMR partial accept, to help reflecting and discussing the feature.
Full discussion: #7309
I'll try to create examples that demonstrate the feature in action when I get some time.
Additional context
I have made the feature optin, so that frameworks that don't need it would spare its computing costs. This option would most probably be controlled by framework specific plugins. In its current form
acceptExportsis compatible and can coexist with the rest of the existing HMR API.The main drawback (I have identified) of the partial accept API is that we need to do a detailed analysis of what is imported from other modules, instead of just which modules are imported. For the POC, I have used parse-static-imports but maybe this can optimized further.
What is the purpose of this pull request?
Currently the main purpose is exploratory.
Before submitting the PR, please make sure you do the following
fixes #123).