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

Bug: (flat config) ESLint crashes when merging a null value in a config #17820

Closed
1 task
bradzacher opened this issue Dec 6, 2023 · 14 comments · Fixed by #17906
Closed
1 task

Bug: (flat config) ESLint crashes when merging a null value in a config #17820

bradzacher opened this issue Dec 6, 2023 · 14 comments · Fixed by #17906
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly repro:yes

Comments

@bradzacher
Copy link
Contributor

bradzacher commented Dec 6, 2023

Environment

Node version: v18.16.1
npm version: v9.5.0
Local ESLint version: v8.55.0 (Currently used)
Global ESLint version: Not found
Operating System: darwin 23.1.0

What parser are you using?

Default (Espree)

What did you do?

module.exports = [
  {
    languageOptions: {
      parserOptions: {
        foo: {},
      }
    },
  },
  {
    languageOptions: {
      parserOptions: {
        foo: null,
      }
    },
  },
]

What did you expect to happen?

The second parserOptions.foo should override the first - which is the way it used to work with .eslintrc configs.

What actually happened?

Oops! Something went wrong! :(

ESLint: 8.55.0

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at deepMerge (<root>/node_modules/eslint/lib/config/flat-config-schema.js:91:30)
    at deepMerge (<root>/node_modules/eslint/lib/config/flat-config-schema.js:105:27)
    at ObjectSchema.merge (<root>/node_modules/eslint/lib/config/flat-config-schema.js:271:16)
    at <root>/node_modules/@humanwhocodes/object-schema/src/object-schema.js:244:54
    at Array.reduce (<anonymous>)
    at ObjectSchema.merge (<root>/node_modules/@humanwhocodes/object-schema/src/object-schema.js:237:24)
    at ObjectSchema.merge (<root>/node_modules/@humanwhocodes/object-schema/src/object-schema.js:176:39)
    at <root>/node_modules/@humanwhocodes/object-schema/src/object-schema.js:244:54
    at Array.reduce (<anonymous>)

Link to Minimal Reproducible Example

https://github.com/bradzacher/eslint-flat-config-bugs/tree/main/null-merge-crash

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

In typescript-eslint we have a convenience config which can be used to disable type-aware linting on files i.e. someone can use { files: ["*.js"], extends: ['plugin:@typescript-eslint/disable-type-checked'] } to override their base config and disable type-aware linting.

In eslintrc files this works as expected. In flat configs this crashes.

@bradzacher bradzacher added bug ESLint is working incorrectly repro:needed labels Dec 6, 2023
@fasttime
Copy link
Member

fasttime commented Dec 6, 2023

Thanks for the report! There happens to be an ongoing discussion about this problem in another issue: #17752 (comment).

@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label Dec 6, 2023
@nzakas
Copy link
Member

nzakas commented Dec 6, 2023

We should definitely be duplicating the merge behavior of eslintrc for this case, so marking as accepted.

@JoshuaKGoldberg
Copy link
Contributor

Should this be blocked on #17752 being merged? That PR also changes the deepMerge function and adds tests. I suspect it'd be easier to wait to implement.

@mdjermanovic
Copy link
Member

#17752 has been merged now.

@fasttime
Copy link
Member

I would like to work on this, as I'm already familiar with the relevant code.

@fasttime
Copy link
Member

I've drafted a PR here to make the way merging works more similar to eslintrc. I've also included a list of cases that would be affected by the changes, which I will keep up-to-date if we decide make more changes. These should make it easier to discuss the details. Please, have a look.

@bradzacher
Copy link
Contributor Author

Question - I see that you have just done a v9 alpha release. Will this fix be released for v8? The reason being that this is blocking us from supporting flat configs and so is also blocking many of our users from switching to flat configs.

Would be good to get this out before v9 so that we can help users migrate ahead of v9 to ease migration pains.

@fasttime
Copy link
Member

AFAIK no new v8 versions are planned. v8.56.0 is expected to be the last one: https://github.com/eslint/tsc-meetings/blob/main/notes/2023/2023-12-14-transcript.md?plain=1#L77-L79.

@bradzacher
Copy link
Contributor Author

bradzacher commented Dec 30, 2023

This is a real problem for us as mentioned. There's a non-trivial number of our users that are stuck then and are thus going to be hit with a really painful migration to v9 as they will have to deal with the flat config migration AND whatever other breaking changes are shipping with the major.

I would strongly urge you guys to reconsider and backport just bug fixes to v8 to aid with migrations.

As mentioned - we can't migrate the @typescript-eslint repo to a flat config because of this bug - which is blocking us from releasing our own flat config support.

It would be great if we could release official support from our project to help people migrate early given that we currently support around 70% of eslint users.

@fasttime
Copy link
Member

@nzakas @mdjermanovic anything we can do to help with the typescript-eslint migration?

@bradzacher
Copy link
Contributor Author

Just pinging back on this in the new year. Is this something we can look into for backporting to v8?

We have users playing around with v9-alpha and finding crashes and we can't even begin to consider handling that until we have flat configs working.

But we can't get flat configs working without the fix.

@fasttime
Copy link
Member

fasttime commented Jan 6, 2024

@bradzacher To request a new v8 release maybe it would be better if you open a new issue. A new latest release would be more work than just backporting that PR given our current setup and workflow. I can't say if that request would be accepted, but clearly the discussion would be more visible.

@woutermont
Copy link

@bradzacher, is this issue only a problem for the convenience config, or does it pop up elsewhere too?

To unblock the stalemate, might it be an idea to temporarily use false instead of null to communicate that disabling override?

@nzakas
Copy link
Member

nzakas commented Jan 17, 2024

@woutermont please see discussion on #17966

@eslint eslint locked as resolved and limited conversation to collaborators Jan 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly repro:yes
Projects
Archived in project
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants