feat(pluginutils): add createHookFilter()
#1916
Conversation
|
hey thanks for the PR. is this something specific for Rolldown? it's the first time I've seen a proposal for the function/need, and I'm not aware of plugins that are filtering on hooks as yet. At least not in this repo. I'd like to get some more information from you before we expand the API of the pluginutils package. |
|
From what I understand this is available in both rollup and rolldown, see https://rollupjs.org/plugin-development/#build-hooks and scroll down to The gist of it is that by passing the filter to the bundler declaratively, the bundler doesn't need to call out to plugins that often, which in the case of rust<->js saves a bit of time. I'd assume that in case of rollup there is no performance difference, as the code just moved from pluginutils to rollup core. edit: I've noticed that I regressed the case when a regex has a sticky flag, I've added two tests + a fix. edi2: I noticed that rollup does post-process patterns for hook filters too, but doesn't allow control how globs are resolved, so I changed things to convert picomatch to regex first, so the patterns are used in rollup as is. |
|
Yes I'm aware of them being supported in Rollup proper. What I'm missing here is this pattern being used in plugins. I know that we don't have any plugins in this repo which are supporting or passing hook filters. Is that something commonly used in rolldown plugins or community plugins where we see the pattern used frequently in an ad hoc manner? What I'm trying to determine is whether this belongs in pluginutils (which is meant to be used by plugins, and not build configs etc) or a third party convenience lib. |
When trying to port a plugin that uses `createFilter()` to hook filters it's hard keep the functionality 100% the same, as `createFilter()` does various pre-processing, like normalising paths and, resolving relative paths, exluding paths which contain \0, etc. To make porting easier, provide a `createHookFilter()` which instead of returning a function, returns a filter object that can be assigned to "filter.id" or "filter.code", and behaves the same as if createFilter() was used and the returned function used in a hook. To make sure that the implementation of `createHookFilter()` matches `createFilter()`, `createFilter()` now calls `createHookFilter()` internally and uses the same object to create the test function.
|
I could see it replacing most This would give better rolldown performance (8% build time reduction for me) with a few lines changed while not changing behavior for users. |
|
I'm sorry but your reply doesn't help to answer the questions that I'm after. A repo-wide refactor for a project which isn't a direct dependent is just out of the question as we simply do not have enough active maintainers. I think at this time it would be more beneficial to create a pluginutils for rolldown specifically, rather than adding that method to the pluginutils package here. |
|
I only use plugins from this repo, so creating an external tool would not help me. Feel free to close then. |
|
Fair enough. The criteria for adding to the pluginutils package is that it's something that's widely used by plugins. At this time, it doesn't fit that criteria. If Rolldown has specific improvements it wants to make and submit PRs to individual plugins, and we end up with repetition across those improvements, then it would be warranted. |
|
It's a bit of a chicken and egg problem imo, plugins will not move to the new API if it's difficult to do so and they have to replicate undocumented pluginutils internals. But I don't maintain any plugins so that's just a guess on my end. |
2 participants
Rollup Plugin Name:
pluginutilsThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Description
When trying to port a plugin that uses
createFilter()to hook filters it's hard keep the functionality 100% the same, ascreateFilter()does various pre-processing, like normalising paths and, resolving relative paths, exluding paths which contain \0, etc.To make porting easier, provide a
createHookFilter()which instead of returning a function, returns a filter object that can be assigned to "filter.id" or "filter.code", and behaves the same as if createFilter() was used and the returned function used in a hook.To make sure that the implementation of
createHookFilter()matchescreateFilter(),createFilter()now callscreateHookFilter()internally and uses the same object to create the test function.This was motivated by me trying to port a a plugin which used
createFilter()to the new API for easy faster rolldown performance, and having a hard time figuring out how to port while making sure that nothing changed for existing users. Not that many new tests added, since all createFilter() calls now also go through the new code, but I can add more if wanted.Note sure if this is something that would be accepted :) (and my typescript is wonky... keep that in mind)