Skip to content

perf(yaml)!: use hook filters#1988

Open
sapphi-red wants to merge 1 commit intorollup:masterfrom
sapphi-red:feat/yaml-use-hook-filters
Open

perf(yaml)!: use hook filters#1988
sapphi-red wants to merge 1 commit intorollup:masterfrom
sapphi-red:feat/yaml-use-hook-filters

Conversation

@sapphi-red
Copy link
Contributor

Rollup Plugin Name: yaml

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no (covered by existing tests)

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • 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

Similar to #1954, but for yaml plugin. This PR adds plugin hook filters for improved performance for Rolldown.
Object style hooks are added by rollup/rollup#4600 which is released in 2.78.0, so I bumped the minimum required Rollup version.

BREAKING CHANGES:
- drops support for rollup below v2.78.0
@sapphi-red sapphi-red requested a review from shellscape as a code owner March 23, 2026 18:30
@shellscape
Copy link
Collaborator

@CharlieHelps please do a thorough review of this PR to determine if it's ready to merge. Look at the diff, reviews, comments, and referenced issues/docs/links to build a complete understanding. Then respond with a comment that includes a list of MUST fix items that must be fixed before the the PR is merged, and a list of IMPROVEMENTS that shouldn't block the merge but may be quick improvements or things that could be addressed in a follow up PR. The MUST fix items should explain exactly what's wrong and what needs to change to resolve the problem. Both should be numbered lists and the IMPROVEMENTS list should continue counting up from the MUST list. It's OK if there are no IMPROVEMENTS or MUST fix items, you can just omit the lists.

@charliecreates
Copy link
Contributor

I did a full pass over the diff, current discussion/reviews, and referenced context (including #1954 and the related suffixRegex behavior in pluginutils). I don’t think this is ready to merge yet.

MUST fix

  1. extensions: [] behavior regressed and now matches every module.
    The change from extensions.some((ext) => id.endsWith(ext)) to suffixRegex(extensions, 'i') causes extensions: [] to produce /$/i, which matches all ids. That means non-YAML files can pass the hook filter/guard path and be parsed as YAML. On master, an empty extensions array matched nothing. To resolve this, preserve no-match semantics for an empty extensions array (or explicitly reject empty arrays with a clear runtime error) and add a regression test in packages/yaml/test/test.js that verifies yaml({ extensions: [] }) does not transform a non-YAML id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants