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.

Sign up for GitHub

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

Jump to bottom

feat: align object interface for transformIndexHtml hook #9669

Merged
merged 6 commits into from Nov 30, 2022
Merged

feat: align object interface for transformIndexHtml hook #9669

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9f8e8b8
feat: rebase
antfu Aug 23, 2022
83bf06e
Update packages/vite/src/node/plugins/html.ts
antfu Aug 24, 2022
a824406
chore: fix error
antfu Aug 24, 2022
0b74ff0
Update packages/vite/src/node/plugins/html.ts
antfu Aug 25, 2022
05c8aca
Update packages/vite/src/node/plugins/html.ts
antfu Aug 25, 2022
c67189b
chore: merge branch 'main' into feat/transform-html
antfu Oct 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/guide/api-plugin.md
View file
Open in desktop
Expand Up @@ -329,7 +329,7 @@ Vite plugins can also provide hooks that serve Vite-specific purposes. These hoo

### `transformIndexHtml`

- **Type:** `IndexHtmlTransformHook | { enforce?: 'pre' | 'post', transform: IndexHtmlTransformHook }`
- **Type:** `IndexHtmlTransformHook | { order?: 'pre' | 'post', handler: IndexHtmlTransformHook }`
- **Kind:** `async`, `sequential`

Dedicated hook for transforming HTML entry point files such as `index.html`. The hook receives the current HTML string and a transform context. The context exposes the [`ViteDevServer`](./api-javascript#vitedevserver) instance during dev, and exposes the Rollup output bundle during build.
Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/plugin.ts
View file
Open in desktop
Expand Up @@ -106,7 +106,7 @@ export interface Plugin extends RollupPlugin {
*
* By default the transform is applied **after** vite's internal html
* transform. If you need to apply the transform before vite, use an object:
* `{ enforce: 'pre', transform: hook }`
* `{ order: 'pre', handler: hook }`
*/
transformIndexHtml?: IndexHtmlTransform
/**
Expand Down
52 changes: 37 additions & 15 deletions packages/vite/src/node/plugins/html.ts
View file
Open in desktop
Expand Up @@ -229,7 +229,9 @@ function handleParseError(
* Compiles index.html into an entry js module
*/
export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
const [preHooks, postHooks] = resolveHtmlTransforms(config.plugins)
const [preHooks, normalHooks, postHooks] = resolveHtmlTransforms(
config.plugins
)
preHooks.unshift(preImportMapHook(config))
postHooks.push(postImportMapHook())
const processedHtml = new Map<string, string>()
Expand Down Expand Up @@ -721,12 +723,16 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
if (s) {
result = s.toString()
}
result = await applyHtmlTransforms(result, postHooks, {
path: '/' + relativeUrlPath,
filename: id,
bundle,
chunk
})
result = await applyHtmlTransforms(
result,
[...normalHooks, ...postHooks],
{
path: '/' + relativeUrlPath,
filename: id,
bundle,
chunk
}
)
// resolve asset url references
result = result.replace(assetUrlRE, (_, fileHash, postfix = '') => {
return (
Expand Down Expand Up @@ -799,6 +805,11 @@ export type IndexHtmlTransformHook = (
export type IndexHtmlTransform =
| IndexHtmlTransformHook
| {
order?: 'pre' | 'post' | null
handler: IndexHtmlTransformHook
/**
* @deprecated renamed to `order`
*/
enforce?: 'pre' | 'post'
transform: IndexHtmlTransformHook
}
antfu marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -850,24 +861,35 @@ export function postImportMapHook(): IndexHtmlTransformHook {

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

for (const plugin of plugins) {
const hook = plugin.transformIndexHtml
if (hook) {
if (typeof hook === 'function') {
postHooks.push(hook)
} else if (hook.enforce === 'pre') {
preHooks.push(hook.transform)
if (!hook) continue

if (typeof hook === 'function') {
normalHooks.push(hook)
} else {
const order = hook.order ?? hook.enforce
patak-dev marked this conversation as resolved.
Show resolved Hide resolved
const handler = hook.handler ?? hook.transform
if (order === 'pre') {
preHooks.push(handler)
} else if (order === 'post') {
postHooks.push(handler)
} else {
postHooks.push(hook.transform)
normalHooks.push(handler)
}
Copy link
Member

Choose a reason for hiding this comment

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

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
]

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
]

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 👍🏼

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

}
}

return [preHooks, postHooks]
return [preHooks, normalHooks, postHooks]
}

export async function applyHtmlTransforms(
Expand Down
5 changes: 4 additions & 1 deletion packages/vite/src/node/server/middlewares/indexHtml.ts
View file
Open in desktop
Expand Up @@ -43,14 +43,17 @@ interface AssetNode {
export function createDevHtmlTransformFn(
server: ViteDevServer
): (url: string, html: string, originalUrl: string) => Promise<string> {
const [preHooks, postHooks] = resolveHtmlTransforms(server.config.plugins)
const [preHooks, normalHooks, postHooks] = resolveHtmlTransforms(
server.config.plugins
)
return (url: string, html: string, originalUrl: string): Promise<string> => {
return applyHtmlTransforms(
html,
[
preImportMapHook(server.config),
...preHooks,
devHtmlHook,
...normalHooks,
...postHooks,
postImportMapHook()
],
Expand Down
4 changes: 2 additions & 2 deletions playground/html/vite.config.js
View file
Open in desktop
Expand Up @@ -35,8 +35,8 @@ module.exports = {
{
name: 'pre-transform',
transformIndexHtml: {
enforce: 'pre',
transform(html, { filename }) {
order: 'pre',
handler(html, { filename }) {
if (html.includes('/@vite/client')) {
throw new Error('pre transform applied at wrong time!')
}
Expand Down