Add preTransform and postTransform hooks

[aleclarson]

Description

I believe it would be best for Vite if #10671 was implemented as a plugin. It would keep inevitable bug reports out of the core repo. It's up to the team to decide if they want to maintain it or let the community handle things. It doesn't matter to me, as long as Vite is able to support other cache implementations. For example, someone might write a remote caching plugin or something with more granular invalidation.

To support #10671 as a plugin, we need some new plugin hooks.

Suggested solution

preTransform

This hook would be called before the following code:

vite/packages/vite/src/node/server/transformRequest.ts

Lines 237 to 242 in 18c71dc

	// transform
	const transformStart = isDebug ? performance.now() : 0
	const transformResult = await pluginContainer.transform(code, id, {
	inMap: map,
	ssr
	})

The first plugin to return a TransformResult object should end the plugin loop and prevent Vite's transform pipeline from running for that file.

The hook would receive the following information:

• id: string
• file: string (the result of cleanUrl(id))
• ssr: boolean
  Since some plugins may run for SSR only
• mode: string
  Since some plugins may run for development/production only
• loadResult: Readonly<LoadResult> | null
  This will be null if no load hook returned a LoadResult so the code was loaded from disk.
• code: string
• map: Readonly<SourceMap> | null

postTransform

This hook would be called before the following code:

vite/packages/vite/src/node/server/transformRequest.ts

Line 281 in 18c71dc

return result

All plugins with a postTransform hook will be called (in parallel?). The return value is ignored.

The hook would receive everything that preTransform does plus the following:

• transformResult: Readonly<TransformResult>
• originalCode: string
  The code before transform hooks were applied.
• originalMap: Readonly<SourceMap> | null
  The sourcemap before transform hooks were applied.

Additional context

Note that these hooks will only run for transformRequest calls (at least for now).

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

[Akryum]

Persistent caching also needs wrapping around pluginContainer.load

[Akryum]

Should I try to change the PR to core changes + a (temporary?) builtin plugin?

[patak-dev]

@Akryum I have been thinking about Alec's proposal and I believe there is merit to it. The cache isn't free either (regarding disk space for example, and complexity as mentioned). We have a team meeting tomorrow, and we'll try to get to this one. Maybe it is better to wait a bit before doing the work. It would be useful though to sketch what the wrapping around load means, are two more hooks needed there?

[bluwy]

I'm likely not understanding the core issue, but wouldn't wrapping pluginContainer.transform hook to inject your own pre/post code, or inject Vite plugins as the first and last in the array during configResolved not solve the issue?

They are hacky, but it reduces the API surface layer. vite-plugin-inspect does similar too. These hooks seem to only solve a specific problem so I don't think it's worth adding them yet, especially that we already have order and enforce options.

[aleclarson]

Persistent caching also needs wrapping around pluginContainer.load

I don't think it does, because you have access to the loadResult in preTransform.

I'm likely not understanding the core issue, but wouldn't wrapping pluginContainer.transform hook to inject your own pre/post code, or inject Vite plugins as the first and last in the array during configResolved not solve the issue?

Not for preTransform, because the transform step calls every plugin. There is no early bailout. We could make it possible, so I guess it depends on which approach we think is clearer in its intent and has less footguns. The nice thing about preTransform and postTransform is that the user never has to worry about plugin order in their config file. I think the ordering safety is paramount and worth the extra API surface.

[bluwy]

There could be multiple plugins using preTransform and postTransform hooks so there would be another ordering issue, which also adds another dimension to transform ordering in general. So I don't think it would enforce ordering safety here.

[aleclarson]

There could be multiple plugins using preTransform and postTransform hooks so there would be another ordering issue, which also adds another dimension to transform ordering in general. So I don't think it would enforce ordering safety here.

While we can't avoid ordering mistakes entirely, it's a lot less likely with preTransform and postTransform hooks in place.

First of all, implementing preTransform as a transform hook isn't possible without having access to loadResult (separately from code/map values) and it feels awkward to expose loadResult to all transform hooks. Not to mention transform hooks are also called for production builds (where loadResult doesn't exist) and in the devHtmlHook function.

If implementing postTransform as a transform hook and an ordering mistake occurs, the cache would be poisoned, which is a poor experience for users. While in the case of an ordering mistake with postTransform hooks, there's no chance for cache poisoning, because the transform result is readonly for postTransform hooks.

[aleclarson]

We could rename these readFromCache and writeToCache to make it clear why they exist.

Alternatively, we could add a server.transformCache option that takes an object like this:

server: {
  transformCache: {
	read(id, { file, ssr, mode, loadResult, ...etc }) {
	},
	write(id, transformResult, { file, originalCode, ...etc }) {
	}
  }
}

This would make it clear that only one cache implementation should exist.

[patak-dev]

@aleclarson we discussed this proposal in today's team meeting and the general consensus was that we should avoid adding new options or hooks at this point. Remote caching in particular isn't a strong enough reason given how fast rebuilding the cache is compared to download times. We still think that it is a good idea though to be prepared in the future to review this decision and your proposal of abstracting the cache logic as a plugin or module is a good idea also for maintainability. Let's go back to discussing with @Akryum in #10671 how to be able to merge it so we can offer this option in Vite 4, and it is as maintainable as possible moving forward.

[aleclarson]

Remote caching in particular isn't a strong enough reason given how fast rebuilding the cache is compared to download times.

Then why does Turborepo have remote caching? 🤔

[patak-dev]

@aleclarson I think some level of remote caching is good, for example for build artifacts that are costly. We discussed this with @juristr not long ago, maybe a good strategy is to start recommending Nx+Vite. Nx is working on better integration with Vite for their next version.

[aleclarson]

It seems that Nx would need the preTransform/postTransform hooks (or the server.transformCache option) in order to safely integrate its remote caching with the dev server, no?

Also, it seems potentially error-prone to have an internal local cache and an external Nx-based cache both being active at the same time.

[patak-dev]

I think we would use Nx for build artifacts and maybe pre-bundled dependencies. That may be a good target that is costly to generate so you could get them from a remote cache. But for each individual module cache in your own source, it may be more performant to generate them than to download them from a remote cache.

[patak-dev]

Let's close this issue. I don't think we should tackle caching with this API until it is more clear how rolldown and vite will end up integrated.