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

fix: fix tsconfig-less check errors, fix @types/eslint incompatibilities, add tests #8460

Merged
merged 4 commits into from Feb 15, 2024

Conversation

bradzacher
Copy link
Member

@bradzacher bradzacher commented Feb 14, 2024

PR Checklist

Overview

This PR addresses a few problems:

  1. Our flat config types are completely incompatible with @types/eslint - meaning plugins defined using those types cannot be used in tseslint.config(..) (Bug: type FlatConfig.Config['files'] do not match with eslint #8467)
  2. Our packages do not support node10 (Docs: getting started guide causes type errors in eslint.config.js #8459)
  3. We had no tests to validate how someone might use typescript-eslint IRL

For (3) I added an integration test which runs TS over a eslint.config.js file and validates the outputs so we can truely validate that typescript-eslint works as intended with no errors.


For (1) I went through and updated the types to align them or loosen them.
We've chatted about this before and I don't love loosening them but there's just severe incompatibilities between the two that means we have no choice really. We're a lot stricter than the DT types to enforce users do the right thing - but DT isn't about enforcing strictness; it's about correctly representing the state of the world.
i.e. the types are just fundamentally incompatible as they serve different use cases!


For (2) ... Users right now are running into a problem:
Our getting started guide tells them to add // @ts-check to their eslint.config.js file.
When they copy+paste the example code into their file they see a type error from TS:

Cannot find module 'typescript-eslint' or its corresponding type declarations.ts(2307)

Playing around I believe the issue is that their eslint.config.js is not included in any tsconfig.jsons. Without a tsconfig.json TS uses the "default" compiler options which sadly means the file is checked with module: node10 😢 (#7284).

So we have a three options really:

  1. update the getting started guide to tell everyone to reconfigure their tsconfig.json so that they have their eslint.config.js included in it
  2. make the package work under node10
  3. remove // @ts-check and don't recommend typechecking of config files

Option (3) I hate. The entire point of tseslint.config was so that we could allow users to have typechecked config files. We have all the properties jsdoc'd so that users get autocomplete with all the documentation in their IDE.
So removing that from the guide is a MASSIVE step back.

Option (1) could work - however it's not going to be easy to write generic and simple docs to tell the user how to cover the eslint.config.js file with a tsconfig. For example if the user has just one tsconfig.json in their repo that they use for their build they need to separate their build config into a tsconfig.build.json so that they can add non-build files to tsconfig.json. Essentially we don't know the user's setup so it's hard to provide that succinct "silver bullet" advice on how you might do things.
Given that we want the getting started guide to be super simple - IMO this makes option (1) a non-starter.

So that means (2) is the best bet 😭. I hate it because as per #7284 we really don't want to do this hack for backwards compat.

@bradzacher bradzacher added the bug Something isn't working label Feb 14, 2024
@typescript-eslint
Copy link
Contributor

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlify bot commented Feb 14, 2024

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit c0b9e62
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/65ce166d2ea26d0008cf4ed4
😎 Deploy Preview https://deploy-preview-8460--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 96
Accessibility: 100
Best Practices: 92
SEO: 98
PWA: 80
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

nx-cloud bot commented Feb 14, 2024

@Zamiell
Copy link
Contributor

Zamiell commented Feb 14, 2024

Also note that in the ultra long term, TypeScript itself would update the default value from Node10 to the newer setting. In that world, this PR could be reverted. So maybe in 5 years or so?

@bradzacher
Copy link
Member Author

@Zamiell - microsoft/TypeScript#54500 - TS 6.0 maybe?
we're on 5.3 - +0.1 per quarter so 7 quarters -- 2 years!?!?

@htho
Copy link

htho commented Feb 14, 2024

Hi, I opened #8459 which is primarily a documentation issue, but it also shows, that type information for tseslint.config() is (partially) missing.

I manually applied "main": "./dist/index.js", to my ./node_modules/typescript-eslint/package.json

The Problem is @typescript-eslint/utils/ts-eslint can not be resolved, and therefore FlatConfig can not be imported.

Maybe "main": "./dist/index.js" has to be applied to other package.json files too?

@bradzacher
Copy link
Member Author

bradzacher commented Feb 14, 2024

Hmm. My little toy tester didn't actually paste the example in - I just had the import and fixed the error on the import so I didn't see any errors inside node_modules!

Will defs need to think on this more 🤔

@bradzacher
Copy link
Member Author

Cc @jakebailey do you know of a way we can help users solve this easily in a consistent, generic way? Or are we just stuck because of the node10 default?

@jakebailey
Copy link
Collaborator

jakebailey commented Feb 14, 2024

Just to be super clear, are you solely worried about the editor case?

In the next version of TypeScript, there's a new mode that vscode will use as its default for the implicit project; module=preserve + moduleResolution=bundler, which will likely make this all go away as that combo will be export map aware.

@bradzacher
Copy link
Member Author

@jakebailey yeah IMO the editor usecase is the primary concern. I think a lot of people won't want to strictly typecheck their config files via the CLI - if they do then this problem goes away!

Using // @ts-check means that they can have type errors and autocomplete in their IDE whilst authoring their config which should be "good enough" given eslint also checks the config at lint-time.

default for the implicit project; module=preserve + moduleResolution=bundler, which will likely make this all go away as that combo will be export map aware

That's great to hear!
But that default will require people to upgrade to TS5.4, right? Or will it apply to vscode's setup by default?

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

👍 makes sense.

Once the experimental project service is stabilized and #8206 resolved, this should no longer be an issue. We then should be able to remove it for v8.

@bradzacher
Copy link
Member Author

Once the experimental project service is stabilized and #8206 resolved, this should no longer be an issue

@JoshuaKGoldberg it won't fix anything! This isn't a problem with ESLint reporting on the file - it's a problem with TypeScript itself reporting on the file!

@bradzacher bradzacher added the DO NOT MERGE PRs which should not be merged yet label Feb 14, 2024
@jakebailey
Copy link
Collaborator

But that default will require people to upgrade to TS5.4, right? Or will it apply to vscode's setup by default?

Yes, people would need to update. Though, a lot of people use the built-in version of TS, which helps.

I'll note that the fact that vscode uses different defaults for the inferred (implicit? one of those) poses the question of what defaults you all should set too; I suspect that you may want to mirror them.

@bradzacher
Copy link
Member Author

bradzacher commented Feb 14, 2024

The big two things we strongly disagree with is

  • the default of node10 module resolution -- we push everyone to node16
  • the default of same strict settings like strictNullChecks -- we push everyone to turn them on.

But the former is the only requirement we've imposed (much to some people's chagrin #7284).

I honestly hate the fact that node10 resolution is the default and will likely remain the default till TS 6.0 because it's a hidden land mine for a lot of usecases.

@jakebailey
Copy link
Collaborator

jakebailey commented Feb 14, 2024

In 5.4+, VS Code will default to module=preserve/moduleResolution=bundler, and it has defaulted to strictNullChecks and strictFunctionTypes for a year or two. I do feel like you'd stand to benefit from also setting those defaults if available; it'll only affect random files not in projects, and these are the settings I'd hazard to say that most people see anyway if they never configured TypeScript. (Or, let them be configurable?)

But, yes, I don't think these defaults are going to change until 6.0 unless we decide to change what we announced about setting stability previously. I'm not totally sure what a better default would be, though; the combo I described above is a perfect case for VS Code as it never emits, and arguably eslint, but it's not a great default for regular tsconfigs as its emit is probably not super helpful.

@bradzacher bradzacher changed the title fix(typescript-eslint): add a main entry to package.json to workaround type errors fix: fix tsconfig-less check errors, fix @types/eslint incompatibilities, add tests Feb 15, 2024
@bradzacher
Copy link
Member Author

@JoshuaKGoldberg this PR has been radically updated from the 1 line PR you stamped.
Please re-review 😄

@bradzacher bradzacher removed the DO NOT MERGE PRs which should not be merged yet label Feb 15, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Yeah 😬 I don't see a way around this. Good work!

packages/utils/src/ts-eslint/Rule.ts Outdated Show resolved Hide resolved
packages/utils/src/ts-eslint/Rule.ts Show resolved Hide resolved
packages/utils/src/ts-eslint/Rule.ts Outdated Show resolved Hide resolved
packages/utils/src/ts-eslint/Parser.ts Outdated Show resolved Hide resolved
packages/utils/src/ts-eslint/Processor.ts Outdated Show resolved Hide resolved
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
@bradzacher bradzacher merged commit 18c3216 into main Feb 15, 2024
61 checks passed
@bradzacher bradzacher deleted the fix-ts-eslint-pkg-type-errors-sigh branch February 15, 2024 14:07
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: getting started guide causes type errors in eslint.config.js
5 participants