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

chore: add PluginWithRequiredHook type & extract getHookHandler function #14845

Merged
merged 4 commits into from
Nov 2, 2023

Conversation

lzt1008
Copy link
Contributor

@lzt1008 lzt1008 commented Nov 1, 2023

Description

This PR mainly refactors the code to extract the common getHookHandler utility to get hook from object style hooks.

The repeated pattern of:

'handler' in hook ? hook.handler : hook

is extracted into getHookHandler with correct TypeScript type to reduce duplication.


This PR also added a WithRequiredHook type to get a more precise plugin type when requiring the specific hook.

Based on this if statement, we can guarantee that all returned Plugins has the hook of hookName.

if (hook) {
if (typeof hook === 'object') {
if (hook.order === 'pre') {
pre.push(plugin)
continue
}
if (hook.order === 'post') {
post.push(plugin)
continue
}
}
normal.push(plugin)
}

We should pass this information on in subsequent code. Equally, getHookHandler's extraction relies on this more precise return type


This PR is a pure refactoring to improve code structure. No behavior or functionality changes.

Additional context

One usage haven't refactor to getHookHandler directly due to its different implementation, I've marked with TODO to align in a follow up.

return typeof hook === 'object' && 'handler' in hook
? hook.handler
: hook

Additionally, The implementation of object style hook in Rollup is:

const handler = typeof hook === 'object' ? hook.handler : hook;

https://github.com/rollup/rollup/blob/fbf806aceffd822d43e4603b664c54165c72cf36/src/utils/PluginDriver.ts#L322

Comparing the two implementations of Vite, which one is the best?

hook === 'object' && 'handler' in hook ? hook.handler : hook 
'handler' in hook ? hook.handler : hook

If someone wrote a plugin like this, Vite will act different with Rollup, should we keep this 'handler' in hook logic?

const plugin = () => {
  const p = {
    resolveId() {
      console.log('call function')
    }
  }
  p.resolveId.handler = function() {
      console.log('call handler')
  }
  return p
}

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 PR Title 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.

Copy link

stackblitz bot commented Nov 1, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev
Copy link
Member

Thanks for the PR! This looks like a great refactoring to me, and it shows some inconsistencies as you described. I think we should change the implementation to match Rollup's version. If the hook is a function, I think it should be called and not go with its .handler even if it is there.

@patak-dev patak-dev added the p1-chore Doesn't change code behavior (priority) label Nov 1, 2023
@lzt1008 lzt1008 changed the title chore: add WithRequiredHook type & extract getHookHandler function chore: add PluginWithRequiredHook type & extract getHookHandler function Nov 2, 2023
@@ -959,7 +960,7 @@ function injectSsrFlagToHooks(plugin: Plugin): Plugin {
function wrapSsrResolveId(hook?: Plugin['resolveId']): Plugin['resolveId'] {
if (!hook) return

const fn = 'handler' in hook ? hook.handler : hook
const fn = getHookHandler(hook)
Copy link
Member

Choose a reason for hiding this comment

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

I was also thinking if we should handle if fn is undefined here, but I suppose the types always restricts that you need a handler with the object-form, so I suppose this is also fine for now.

@patak-dev patak-dev merged commit 997f2d5 into vitejs:main Nov 2, 2023
11 checks passed
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants