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.

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

refactor: [hookName].handler in plugins #19586

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Mar 6, 2025

Description

Moves some plugin[hookName] to plugin[hookName].handler so that we can add filter without a big diff.

This PR has a big diff, but it only moves the hooks under .handler so it shouldn't change any behavior unless the 3rd party plugin / framework calls the plugin manually.

@sapphi-red sapphi-red added the p1-chore Doesn't change code behavior (priority) label Mar 6, 2025
Comment on lines +446 to +450
// for backward compat, make `plugin.transform` a function
// but still keep the `handler` property
// so that we can use `filter` property in the future
plugin.transform = transformHook.handler
;(plugin.transform as any).handler = transformHook.handler
Copy link
Member Author

Choose a reason for hiding this comment

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

@sapphi-red
Copy link
Member Author

/ecosystem-ci run

Copy link

pkg-pr-new bot commented Mar 6, 2025

Open in Stackblitz

npm i https://pkg.pr.new/vite@19586

commit: 273a6d2

@vite-ecosystem-ci

This comment was marked as outdated.

patak-dev
patak-dev previously approved these changes Mar 6, 2025
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Maybe we should merge this one in Vite 6.3, just to play safe. But I'm ok if you'd like to go with it in a patch to avoid blocking other work.

The diff is massive. Adding a filter would be a common pattern in the future, and I think the boat already sailed but I wonder if it would have been better to keep the hooks as function only and add new properties for each to configure them. It would also have avoided all the indirections to deal with the hook being an object or a function.

transformConfig: {
  filter: /x/,
  order: 'pre'
}
transform(code, id) {
  return transformed(code)
}

@sapphi-red
Copy link
Member Author

Maybe we should merge this one in Vite 6.3, just to play safe. But I'm ok if you'd like to go with it in a patch to avoid blocking other work.

I'm fine with merging in 6.3 👌

The diff is massive. Adding a filter would be a common pattern in the future, and I think the boat already sailed but I wonder if it would have been better to keep the hooks as function only and add new properties for each to configure them. It would also have avoided all the indirections to deal with the hook being an object or a function.

Yeah, maybe that would have made some places easier to handle.

@patak-dev patak-dev added this to the 6.3 milestone Mar 6, 2025
@sapphi-red sapphi-red force-pushed the refactor/use-hookname-handler branch from 9ea022a to 576695a Compare March 6, 2025 09:53
@sapphi-red
Copy link
Member Author

I rebased this PR on top of #19588 as that one should be safe to merge in a patch and this PR conflicts with that one.

@sapphi-red sapphi-red force-pushed the refactor/use-hookname-handler branch from 576695a to 6566f12 Compare March 7, 2025 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1-chore Doesn't change code behavior (priority) trigger: preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants