plugins icon indicating copy to clipboard operation
plugins copied to clipboard

fix(pluginutils): only normalize/parse patterns once in createFilter

Open lazka opened this issue 2 months ago • 2 comments

Rollup Plugin Name: pluginutils

This PR contains:

  • [ ] bugfix
  • [ ] feature
  • [ ] refactor
  • [ ] documentation
  • [x] other

Are tests included?

  • [ ] yes (bugfixes and features will not be merged without tests)
  • [x] no

Breaking Changes?

  • [ ] yes (breaking changes will not be merged unless absolutely necessary)
  • [x] no

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 passing patterns to createFilter() it would normalize and parse the patterns on every call to the returned filter function.

Just do it once at the start instead.

Something I've noticed while working on #1916.

lazka avatar Oct 21 '25 07:10 lazka

Thanks for the review. Sadly I don't quite understand what type of test you have in mind. Do you want me to move getMatcher to the module level and export it, and then test it? Though it still behaves the same, so there is no way to test the behaviour change.

Adding a spy on picomatch would makes sense, but I couldn't find a way to do that.

lazka avatar Oct 30 '25 18:10 lazka

No worries. Your PR is changing the code so that instead of creating the matcher on every invocation of test, it's creating it once in the scope above, and every invocation of test uses the same matcher. To prevent against regressions of that change, we need a test in place which asserts that test is using the single matcher. Spitballing here - that could be as simple as exporting getMatcher individually, and using a symbol attached to the pattern to assert that each invocation of the resulting test method is using the same one. (It might be enough to copy and past the above to an agent and see what one comes up with)

Changes like this one are very susceptible to regressing in a project like this. We use tests to prevent that from happening.

shellscape avatar Nov 03 '25 14:11 shellscape