Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
feat: align object interface for
transformIndexHtml
hook #9669feat: align object interface for
transformIndexHtml
hook #9669Changes from 1 commit
9f8e8b8
83bf06e
a824406
0b74ff0
05c8aca
c67189b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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, andpost
by default) toorder
(with three values, the same as with the regular plugins).Maybe we should convert
enforce: 'post'
andenforce: undefined
toorder: post
and we make the new default fororder: undefined
work like in this PR (still a post hook, but before all 'post' hooks)There was a problem hiding this comment.
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.There was a problem hiding this comment.
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'
tonormalHooks
? (still maporder: 'post'
topostHooks
)The migration will be counterintuitive, but we can avoid this behavior change.
enforce
/order
enforce: 'pre'
enforce: 'post'
order: 'pre'
order: 'post'
There was a problem hiding this comment.
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?
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.@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 addenforce: 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 wireorder
in the exact same way we have now.There was a problem hiding this comment.
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
andorder
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 withenfore: 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 twopost
hooks.There was a problem hiding this comment.
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'
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:
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.
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.There was a problem hiding this comment.
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) andenforce: 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 👍🏼There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?