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

fix: html env replacement plugin position #12404

Merged
merged 2 commits into from
Mar 15, 2023
Merged

fix: html env replacement plugin position #12404

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2f0b4c
fix: html env replacement plugin position
patak-dev Mar 13, 2023
39d62f4
chore: htmlEnvHook after preHooks
patak-dev Mar 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/vite/src/node/plugins/html.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,8 +285,7 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
const [preHooks, normalHooks, postHooks] = resolveHtmlTransforms(
config.plugins,
)
preHooks.unshift(preImportMapHook(config))
normalHooks.unshift(htmlEnvHook(config))
preHooks.unshift(htmlEnvHook(config), preImportMapHook(config))
Copy link
Member

Choose a reason for hiding this comment

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

The idea is that user pre-hook plugins can run before htmlEnvHook like in dev:

vite/packages/vite/src/node/server/middlewares/indexHtml.ts

Lines 53 to 59 in 42e0d6a

preImportMapHook(server.config),
...preHooks,
htmlEnvHook(server.config),
devHtmlHook,
...normalHooks,
...postHooks,
postImportMapHook(),

Maybe we should push htmlEnvHook to preHooks instead of unshift?

Copy link
Member Author

Choose a reason for hiding this comment

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

You would like this so users are able to inject new env placeholders in the HTML to be replaced?

It makes the feature more powerful for users but I'm afraid of the burden we would impose on plugin authors. preHooks added by plugins would need to take into account that in a URL attr there could now be a %VITE_URL% placeholder. And that means they can't process all URLs. That is why I added the first plugin that I felt was safer and easier to explain.

I'm not sure what is best here, the position of the plugin changes quite a bit the way the feature works.

Copy link
Member

@bluwy bluwy Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You would like this so users are able to inject new env placeholders in the HTML to be replaced?

Yeah, or if they want to "see" the raw HTML before Vite replaces it.

It makes the feature more powerful for users but I'm afraid of the burden we would impose on plugin authors. preHooks added by plugins would need to take into account that in a URL attr there could now be a %VITE_URL% placeholder. And that means they can't process all URLs. That is why I added the first plugin that I felt was safer and easier to explain.

transformIndexHtml ordering is based on it's order/enforce option, not the plugin's enforce option though, so a plugin author would only access this if they have transformIndexHtml.order: 'pre', which means they explicitly opt-in to this pre-behaviour.

I think it's good that we carve out an API for "before env replacement" since I think there's a good chance some plugins want to tap into this phase.

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 agree it is good to have the option to slide before the env replacement. But we would also lose the option to go after it. For example to process a URL in an attr that was replaced. I think both have cons and pros, I'm fine with either.

I was afraid we were adding this in a minor, so we could break plugins. But this isn't a big issue as no one is actually using this yet so some plugins may break when users start to use the feature but it isn't really a breaking change per-se.

Copy link
Member

Choose a reason for hiding this comment

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

I guess you do have a point too which I've not thought before, and there would always be a tradeoff unless we add a new "almost-normal" hook 😛 In the case of plugins processing URLs, they could still workaround by manually replacing %...% themselves (read from resolvedConfig.env), so it might not be a big blocker. Happy to hear what others think too though.

Copy link
Member

@antfu antfu Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think enforce: 'pre' should be able to get the "raw content" just like how transform hook works, that we don't resolve the path etc. If users want to have the HTML after Vite, they could use the normal or post hook.

Copy link
Member

Choose a reason for hiding this comment

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

When using pre, one can't assume the input is a valid format (like pre-transform it can get raw CSS, raw Vue, or anything). This would allow plugins to provide preprocessor or actually inject %...% syntax for Vite to interop etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

@antfu in this case, we would resolve the paths in both options after the envPlugin. So the question is not if it should go before or after Vite processing, but if it should go before or after user hooks with enforce: 'pre'

Option a)

  • env replacement
  • user pre hooks
  • vite processing (resolving paths, etc)
  • user normal and post hooks

Pros: pre hooks will see the replaced values and can transform them before Vite processing
Cons: pre hooks can't inject %VITE_VAR% into the HTML (but they shouldn't need, they have access to the env themselves to inject them)

Option b)

  • user pre hooks
  • env replacement
  • vite processing
  • user normal and post hooks

Pros: see the raw HTML including %VITE_VAR% placeholders
Cons: plugin authors need to care that pre hooks could see an href attr with %VITE_VAR%, so they need to guard against it if they are processing URLs (the test case added in this PR)

The more I think about it, I'm leaning towards a) 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I didn't see your second message before sending mine. Good to see you aren't worried about having the guards from a plugin author's perspective. Points for b) then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Modified the PR to use option b) so we can move forward with the release. If you both think this is better, let's go with it 👍🏼

postHooks.push(postImportMapHook())
const processedHtml = new Map<string, string>()
const isExcludedUrl = (url: string) =>
Expand Down
1 change: 1 addition & 0 deletions playground/html/.env
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
VITE_FOO=bar
VITE_FAVICON_URL=/sprite.svg
5 changes: 5 additions & 0 deletions playground/html/__tests__/html.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,11 @@ describe('env', () => {
expect(await page.textContent('.env-bar')).toBeTruthy()
expect(await page.textContent('.env-prod')).toBe(isBuild + '')
expect(await page.textContent('.env-dev')).toBe(isServe + '')

const iconLink = await page.$('link[rel=icon]')
expect(await iconLink.getAttribute('href')).toBe(
`${isBuild ? './' : '/'}sprite.svg`,
)
})
})

Expand Down
1 change: 1 addition & 0 deletions playground/html/env.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
<p class="env-%VITE_FOO%">class name should be env-bar</p>
<p class="env-prod">%PROD%</p>
<p class="env-dev">%DEV%</p>
<link rel="icon" href="%VITE_FAVICON_URL%" />