Skip to content

Comments

fix: merge overrides with the same file patterns#244

Merged
Sysix merged 7 commits intooxc-project:mainfrom
connorshea:fix-overrides
Nov 21, 2025
Merged

fix: merge overrides with the same file patterns#244
Sysix merged 7 commits intooxc-project:mainfrom
connorshea:fix-overrides

Conversation

@connorshea
Copy link
Member

@connorshea connorshea commented Nov 14, 2025

I created a very minimal reproduction test for this issue, and I believe I've fixed it, but it may have other consequences that need to be figured out and resolved. I'm pretty confident in this now

This was heavily AI-assisted after adding the tests, so I need to test it further, but based on the snapshots it seems like a pretty good solution.

See #240 for more info.

As an example of how much this fixes, I've been using the mastodon repo as a testing ground for the migrate package:

Results from running `yarn run oxlint` on the mastodon codebase after migrating it with `npx @oxlint/migrate eslint.config.mjs`

### v1.28.0 migrate:

Found 2540 warnings and 465 errors.

### main branch of migrate + overrides fix:

Found 67 warnings and 474 errors.

### From main migrate + overrides fix + main oxlint:

Found 67 warnings and 225 errors.

},
},
{
files: ['**/*.ts', '**/*.tsx'],
Copy link
Member Author

Choose a reason for hiding this comment

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

this is kept because it is just one pass of the various steps, so we haven't removed files-only overrides yet.

@connorshea
Copy link
Member Author

connorshea commented Nov 15, 2025

Here's an eslint config:

export default tseslint.config([
  react.configs.flat['jsx-runtime'],
  {
    plugins: {
      "react": react,
      '@typescript-eslint': tseslint.plugin,
    },
    languageOptions: {
      parser: tseslint.parser,
      parserOptions: {
        ecmaFeatures: {
          jsx: true,
        },
      },
    },
    rules: {
      'react/jsx-no-target-blank': [
        'error',
        {
          // Default value for this is false
          allowReferrer: true,
        },
      ],
    },
  },
  {
    files: ['**/*.ts', '**/*.tsx'],
    rules: {
      'react/jsx-no-target-blank': 'error'
    },
  },
]);

react/jsx-no-target-blank enforces that links with target='_blank' also have ``rel='noreferrer'` to avoid tracking and whatnot.

So, what happens when I have this code in app/javascript/mastodon/features/ui/components/link_footer.tsx, which does not have noreferrer?:

<a href='https://joinmastodon.org' target='_blank' rel='noopener'>
  <FormattedMessage id='footer.about' defaultMessage='About' />
</a>

The second config object inherits the base configuration's config for this rule, and so allows excluding noreferrer in tsx files lmao 😭

oxlint (or oxlint-migrate's interpretation of the eslint config I guess idk) does not do this. So that's one source of difference here.

@Sysix
Copy link
Member

Sysix commented Nov 15, 2025

Thank you for working on this!

oxlint (or oxlint-migrate's interpretation of the eslint config I guess idk) does not do this. So that's one source of difference here.

Does it mean that the second block does not really override the settings, but only the severity (in ESLint)?
When ESLint does it but oxlint not, I would guess this is a bug in oxlint? And should later than be patched here too .)

@connorshea
Copy link
Member Author

Does it mean that the second block does not really override the settings, but only the severity (in ESLint)? When ESLint does it but oxlint not, I would guess this is a bug in oxlint? And should later than be patched here too .)

Yeah, in ESLint it only overrides the severity and is probably, technically a bug in oxlint's config resolution behavior, unfortunately.

@connorshea connorshea marked this pull request as ready for review November 15, 2025 21:34
@connorshea connorshea changed the title WIP: Fix overrides bug Fix overrides bug Nov 16, 2025
@connorshea
Copy link
Member Author

After testing this on a decent number of configs, I think this is pretty good and ready to go. We still have the issue described above, but I think that's out-of-scope for this fix for now.

@connorshea
Copy link
Member Author

@Sysix ready for review when you have time :)

@Sysix Sysix changed the title Fix overrides bug fix: merge overrides with the same file patterns Nov 21, 2025
@Sysix Sysix merged commit 5ab97bf into oxc-project:main Nov 21, 2025
2 checks passed
@Sysix
Copy link
Member

Sysix commented Nov 21, 2025

Thank you for the work (and the wait)! :)

@connorshea
Copy link
Member Author

No worries, thank you!

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