feat: align object interface for transformIndexHtml hook

[antfu]

Description

Continues #9634

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.
• 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.

[patak-dev]

We can wait until this week team meeting before merging this PR and #9670, but I think we all wanted to go forward with these changes from the discussions on the RFC.

[sapphi-red]

I think this is a breaking change. Though, I'm not sure how we can avoid this.

For example:

const plugins = [
  { name: 'a', transformIndexHtml: { enforce: 'post', transform: () => { console.log('a') } } },
  { name: 'b', transformIndexHtml: () => { console.log('b') } }
]

// before
const hooks = [
  () => { console.log('a') }, // postHooks
  () => { console.log('b') } // postHooks
]

// after
const hooks = [
  () => { console.log('b') }, // normalHooks
  () => { console.log('a') } // postHooks
]

[patak-dev]

Good catch! I like the idea of enforce (with two values, and post by default) to order (with three values, the same as with the regular plugins).
Maybe we should convert enforce: 'post' and enforce: undefined to order: post and we make the new default for order: undefined work like in this PR (still a post hook, but before all 'post' hooks)

[antfu]

I feel that we should align its behavior with the rest of the hooks. Where we could consider Vite's transformation as a core plugin. Having this would allow plugins to set post to move after normal hooks. I don't really consider this a breaking change as it follows the intuitive and aligns with other hooks and plugin order.

[sapphi-red]

Yeah, I think it's better to ailgn with the rest of the hooks.

I've come up with an idea now.
How about mapping enforce: 'post' to normalHooks? (still map order: 'post' to postHooks)
The migration will be counterintuitive, but we can avoid this behavior change.

	before	after
no enforce/order	2	2
enforce: 'pre'	1	1
enforce: 'post'	2	2
order: 'pre'	-	1
order: 'post'	-	3

const plugins1 = [
  { name: 'a', transformIndexHtml: { enforce: 'post', transform: () => { console.log('a') } } },
  { name: 'b', transformIndexHtml: () => { console.log('b') } }
]
const plugins2 = [
  { name: 'a', transformIndexHtml: { order: 'post', transform: () => { console.log('a') } } },
  { name: 'b', transformIndexHtml: () => { console.log('b') } }
]

// before
const hooks1 = [
  () => { console.log('a') }, // postHooks
  () => { console.log('b') } // postHooks
]

// after
const hooks1 = [
  () => { console.log('a') }, // normalHooks
  () => { console.log('b') } // normalHooks
]
const hooks2 = [
  () => { console.log('b') }, // normalHooks
  () => { console.log('a') } // postHooks
]

[patak-dev]

Would you explain a bit more about your proposal @sapphi-red?

I don't really consider this a breaking change as it follows the intuitive and aligns with other hooks and plugin order.

Before we only had two values, and the default was 'post', so it is somewhat of a breaking change. That said, I doubt it will break any code.

I feel that we should align its behavior with the rest of the hooks. Where we could consider Vite's transformation as a core plugin. Having this would allow plugins to set post to move after normal hooks.

@antfu I also would like the same. Since we are doing this change, maybe we could review the default for normal plugins? Thinking about the transforms that Vite does as a plugin, I think that order: undefined should go before them, and not after. I always found it strange to have to add enforce: pre to add a script for example... I think normal plugins should go before the main Vite pipeline (especially during build)

What about:

• enforce: 'pre' -> order: 'pre'
• enforce: undefined -> order: 'post'
• enforce: 'post -> order: 'post'

Hooks order:
order: 'pre' => order: undefined (normal) => Vite internal => order: post

This isn't a breaking change, since enforce will wire order in the exact same way we have now.

[antfu]

I think @sapphi-red are proposing the same idea to you, by treating enforce and order differently. I agree this would indeed make the behavior strictly the same as previous, but my question is if that really affects any real-world usage or if we are over thinking?

I am not sure if any plugin would use enforce: undefined as in that case they can directly use function hook instead of object hook. And even with enfore: post, we don't guarantee the exact order of each plugin, but that relies on how user-defined it. I would think a good plugin should not have too much constraint on the order of two post hooks.

[sapphi-red]

I think my idea is different from @patak-dev's one.

• enforce: 'pre' -> order: 'pre'
• no enforce(directly using a function) / enforce: undefined -> order: undefined (normal)
• enforce: 'post' -> order: undefined (normal)

To make it clear, my proposal's implementation will be:

export function resolveHtmlTransforms(
  plugins: readonly Plugin[]
): [
  IndexHtmlTransformHook[],
  IndexHtmlTransformHook[],
  IndexHtmlTransformHook[]
] {
  const preHooks: IndexHtmlTransformHook[] = []
  const normalHooks: IndexHtmlTransformHook[] = []
  const postHooks: IndexHtmlTransformHook[] = []

  for (const plugin of plugins) {
    const hook = plugin.transformIndexHtml
    if (!hook) continue

    if (typeof hook === 'function') {
      normalHooks.push(hook)
    } else {
      const order = hook.order ?? hook.enforce
      // @ts-expect-error union type
      const handler = hook.handler ?? hook.transform

      if (order === 'pre') {
        preHooks.push(handler)
      } else if (order === 'post') {
        // CHANGE START
        if (hook.order === undefined) {
          normalHooks.push(handler) // treat `enforce: 'post'` as normal hook
        } else {
          postHooks.push(handler)
        }
        // CHANGE END
      } else {
        normalHooks.push(handler)
      }
    }
  }

  return [preHooks, normalHooks, postHooks]
}

Also I'm not changing the hooks order:
order: 'pre' => Vite internal => order: undefined(normal) => order: post

So this is renaming the current post hook as normal hook and introducing a new post hook.

IIUC @patak-dev's idea is to introduce a new normal hook.
I think your one is still a breaking change because when using function directly, it will change from post hook to normal hook.

my question is if that really affects any real-world usage or if we are over thinking?

If two plugins (one is using function directly and one is enforce: 'post') are injecting script tags, this will be a problem because the order of injected script matters. Though, I'm not sure if there's a real-world usage of this.

[patak-dev]

Now I see that my proposal to change the default for the new order to be before the internal Vite hook isn't possible, sorry for the noise. And I think I understand now why you were fine with the small possibility of a breaking change.

But looks like @sapphi-red's proposal would work. Both enforce: undefined (or function form) and enforce: post are mapped to the same value (that is what I was trying to propose), so there is no breaking change. I think we should go with it 👍🏼

[antfu]

While I agree that @sapphi-red's proposal could be a solution, I am just not sure if the complexity is necessary here. As the difference is so subtle (and technically any change could be considered breaking if others rely on the behavior), maybe we could run a ecosystem CI and see if it affects any known integration before making the change?

[patak-dev]

Isn't a change in the way enforce is interpreted enough? See here https://github.com/vitejs/vite/pull/9669/files#r954987207. Or maybe I'm missing some other complexity for making this work?

[patak-dev]

Going back to the idea of aligning order in this hook with the way order works in other plugins (failed attempt here). After this PR, we will have:

transformIndexHtml hook:

• 'pre' -> Vite internal + build HTML transforms -> normal -> 'post'

for the rest of the hooks:

• 'pre' -> Vite pre plugins -> normal -> Vite build transforms -> 'post'

Maybe we are missing a chance to make 'order' more aligned here? We can't do it under the current hook name, but we have discussed in the past that the name isn't generic enough.

If we rename the hook to transformHtmlEntry, we could define that 'order' could work as:

• 'pre' -> Vite internal -> normal -> build HTML transforms -> 'post'

Maybe Vite internal doesn't have much now for this hook, but we have the chance to add things there if needed later. I care more about having the default apply before the HTML has been transformed (avoiding the need to use enforce: 'pre' in many cases).

If we decide this is a good idea, we could still merge this PR, although it may be a bit confusing and we may want to directly make the jump to the new hook name.

[sapphi-red]

I am for that idea. If we are going with transformHtmlEntry, I think leaving transformIndexHtml as-is is better.

[patak-dev]

For reference, we discussed in a prev team meeting the possibility of having normal hooks be run before some of the build transformation but decided against it as we didn't find a good use case to justify the added complexity.