fix(hmr): don't bind hmr methods to the context class

[sheremet-va]

Description

Fixes #15667

Instead of rewriting the context back to the object implementation, I decided it would be nice to keep the class for organizational purposes. Providing methods via an arrow function allows us to use this without binding the method because it will be transformed into something like this:

class Context {
  constructor() {
    this.accept = () => {
      this.hmtClient.send()
    }
  }
}

Additional context

---

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, especially the Pull Request Guidelines.
• 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).
• Update the corresponding documentation if needed.
• Ideally, include relevant tests that fail without this PR but pass with it.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[patak-dev]

This means that all the functions needs to be created for each new HMRContext. I don't think we should support auto binding of import.meta.hot functions. That was never documented. This may be a good opportunity to explicitly define that projects needs to bind the functions if they need that. See also:

• perf: do not bind plugin hook context functions #15610

[sheremet-va]

See also:

I don't see how this is related. We don't call bind explicitly, and I don't think defining a field has a performance impact different from defining a method.

[patak-dev]

I updated the perf.link that we were discussing in #15610 to show the difference in performance. Using fields has almost the same perf as binding each function in the constructor.

[sheremet-va]

I updated the perf.link that we were discussing in #15610 to show the difference in performance. Using fields has almost the same perf as binding each function in the constructor.

This link shows that to notice a difference you need to load at least 2 million files (context is called once in a file) a second. I think DX would be more important in that case.

[patak-dev]

A new hot context is created for each module that has HMR in the browser. Using vite-dev-server-perf, the effect of binding the context functions for the plugin context was ~1.2% of the total work done by the server. The effect was visible for the 10k modules there. So it should be similar extra work that the browser needs to do here. These functions are used by plugin authors and I don't think they need the extra DX of not needing to auto-bind them, it doesn't seems common enough to pass these functions around too. Vite 5 is working everywhere, and this is the first issue we get about this. I would consider this a feature request more than a fix too.

[sheremet-va]

The effect was visible for the 10k modules there. So it should be similar extra work that the browser needs to do here.

I don't think they can be compared equally. The code in the server runs in the same thread while this code runs in the new realm every time. To measure performance, we would need an actual browser benchmark.

Vite 5 is working everywhere, and this is the first issue we get about this.

Wasn't this change part of 5.1?

I would consider this a feature request more than a fix too.

I don't see how this is a feature request. Internal implementation changed and it affected public usage - this is a regression. TypeScript allows both usages for example.

[patak-dev]

The linked issue said "since v5" so I misread that part. It seems initially we had all the functions working without this, but since v3.2, import.hot.invalidate() started using this.

Given the above and that being able to use the functions without the import.meta.hot was never documented, I would still consider this a feature request. And even if the perf hit will be low, I think we should have a better use case to justify adding this.

[sheremet-va]

The linked issue said "since v5" so I misread that part. It seems initially we had all the functions working without this, but since v3.2, import.hot.invalidate() started using this.

If it was working like this before the refactor, then I am fine to keep it as a feature request rather than a bug.

[bluwy]

Another point: Vite's import.meta.hot.accept syntax works best if it can be statically analyzable from the code:

vite/packages/vite/src/node/plugins/importAnalysis.ts

Lines 455 to 465 in 93fce55

	const importAcceptedUrls = (orderedAcceptedUrls[index] =
	new Set<UrlPosition>())
	if (
	lexAcceptedHmrDeps(
	source,
	source.indexOf('(', endHot + 7) + 1,
	importAcceptedUrls,
	)
	) {
	isSelfAccepting = true
	}

Not that this is a great counter-argument since that Vite shouldn't had relied on this (because I believe webpack was able to figure it out), but we'd need a significant refactor to move the module graph to the browser-side when computing HMR propagation.

---

Either way since Patak pointed out that invalidate had also used this, I think we can require a .bind for now.

[fabiospampinato]

it doesn't seems common enough to pass these functions around too

Fair, but it's also worth pointing out that the current APIs are borderline unusable manually, like there are multiple layers of problems between being able to write something like export default hmr(import.meta.hot, MyComponent) and this code actually working, for no real strong reason I feel, like I don't see why Vite shouldn't just support this scenario.

Either way using arrow functions here would cause a bit more memory to be used, it would be bad if arrays or plain objects wasted memory like that, but I doubt that for the purposes of hmr this would make any significant dent.

Maybe the "right" solution here is that the static analysis should be loosened up and it should just look for import.meta.hot rather than import.meta.hot.accept specifically? Like I'm only passing around import.meta.hot.accept in the first place because Vite needs to see that for some reason, if that problem didn't exist the problem I hit with this binding would also disappear with it, presumably.

[bluwy]

Closing this for now as I think the consensus here is that this is acceptable for now. Vite's HMR implementation is not perfect, static analysis is required for now for it to work nicely, and is also documented:

https://vitejs.dev/guide/api-hmr.html#hot-accept-cb

Vite requires that the call to this function appears as import.meta.hot.accept( (whitespace-sensitive) in the source code in order for the module to accept update. This is a requirement of the static analysis that Vite does to enable HMR support for a module.

We could refactor this to fix it in the future, but it's not an easy task.

[fabiospampinato]

static analysis is required for now for it to work nicely, and is also documented:

Yes, but what I'm saying is: why look for import.meta.hot.accept rather than just import.meta.hot?

We could refactor this to fix it in the future, but it's not an easy task.

Presumably looking for import.meta.hot rather than import.meta.hot.accept should be a trivial change, since detecting import.meta.hot.accept implies already detecting import.meta.hot.

[bluwy]

As linked in my comment above (#15679 (comment)), we need to lex the accepted deps. It's not only used to inject the hot context.

[fabiospampinato]

Makes sense, but it's worth refining the logic behind this imo, because if I'm writing hmr(import.meta.hot) it won't work, if I write hmr(import.meta.hot.accept) it would work, but in neither of those cases I'm calling the function yet, so explicitly writing .accept isn't here isn't telling Vite how I'm going to call that exactly, if that's fine to do today then presumably it should just work the same with import.meta.hot.