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

feat(hmr): experimental.hmrPartialAccept #7324

Merged
merged 18 commits into from
Jun 20, 2022
Merged

Conversation

rixo
Copy link
Contributor

@rixo rixo commented Mar 15, 2022

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 acceptExports is 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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Currently the main purpose is exploratory.

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

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit 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.

@Niputi Niputi added enhancement New feature or request feat: hmr p2-to-be-discussed Enhancement under consideration (priority) labels Mar 15, 2022
@mkilpatrick
Copy link

Looks like you need to add @types/parse-static-imports

@rixo
Copy link
Contributor Author

rixo commented Mar 25, 2022

Looks like you need to add @types/parse-static-imports

@mkilpatrick Unfortunately, the types don't exist apparently...

Anyway, I don't believe that parse-static-imports will make the final cut. I used it for the POC, but I expect quite a bit of discussion around what to use to parse imports, because that's the most meaningful change of the PR, in terms of the project's weight and performance.

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.

@mkilpatrick
Copy link

mkilpatrick commented Mar 25, 2022

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.

@patak-dev
Copy link
Member

patak-dev commented Mar 25, 2022

@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 patak-dev added this to the 3.0 milestone Mar 25, 2022
@patak-dev patak-dev added p2-nice-to-have Not breaking anything but nice to have (priority) and removed p2-to-be-discussed Enhancement under consideration (priority) labels Mar 25, 2022
@rixo
Copy link
Contributor Author

rixo commented Mar 28, 2022

@patak-dev That's very good news!

To be able to move forward, we could get the PR to a state where the performance impact could be measured.

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?

Comment on lines 102 to 119
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)
}
}
Copy link
Member

@bluwy bluwy Apr 11, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

@rixo rixo marked this pull request as ready for review April 11, 2022 23:01
@rixo
Copy link
Contributor Author

rixo commented May 10, 2022

@patak-dev Any idea what would be the next step to help move this forward?

@patak-dev
Copy link
Member

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.

@patak-dev
Copy link
Member

@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 experimental.hmrPartialAccept option defaulting to false? We can then safely include it in v3 and take it out of experimental after a few minors once we know that the API and the perf implications are ok for svelte (and potentially others)

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.

Went through the code and all looks fine to me (though I'm no HMR expert, the changes make sense). After a rebase and with patak's suggestion of the experimental option, this should be good to go. Let me know if you need help with rebasing this 🙂

@bluwy bluwy changed the title wip: HMR partial accept feat(hmr): implement partial accept Jun 19, 2022
@rixo
Copy link
Contributor Author

rixo commented Jun 19, 2022

Sure, I'm on it. Thanks a lot :)

@rixo
Copy link
Contributor Author

rixo commented Jun 20, 2022

I have merged the latest main and renamed the option to experimental.hmrPartialAccept. Also fixed a few glitches with the tests, which should hopefully be stable in CI now.

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.

@patak-dev
Copy link
Member

@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 patak-dev changed the title feat(hmr): implement partial accept feat(hmr): experimental.hmrPartialAccept Jun 20, 2022
@patak-dev patak-dev merged commit 83dab7e into vitejs:main Jun 20, 2022
@patak-dev
Copy link
Member

We're looking for feedback about stabilizing this feature:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feat: hmr p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants