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

Support for typescript-eslint v8's parser projectService #1911

Open
Tracked by #203
tetarchus opened this issue Aug 3, 2024 · 12 comments
Open
Tracked by #203

Support for typescript-eslint v8's parser projectService #1911

tetarchus opened this issue Aug 3, 2024 · 12 comments
Labels
info-needed Issue requires more information from poster

Comments

@tetarchus
Copy link

tetarchus commented Aug 3, 2024

Versions:

eslint: 9.4.0
typescript: 5.4.5
typescript-eslint: 8.0.0
vscode-eslint: 3.0.10 and 3.0.11(pre-release)

Issue

I'm not sure whether this should be raised with the typescript--eslint team, or with the VSCode extension, or whether it's a little of both (or something that I'm doing wrong). I can't seem to find any mention of it on either repo though.

We've been using the v8 alpha-30 release to make use of ESLint v9 and have now switched to using the full release.
When using the projectService option for typescript-eslint's parser in v8.0.0, creating a new file causes the extension to produce the error:

Parsing error: [path-to-newly-created-file] was not found by the project service. Consider either including it in the tsconfig.json or including it in allowDefaultProject.

Requiring that the extension be restarted to force the project service to pick up the new file.
The alpha version (30) of v8 previously raised This rule requires the 'strictNullChecks' compiler option to be turned on to function correctly. on the newly created file, likely caused by the same issue (the project service was not aware of the new file as from the command line it will be started after the file is created).

Background

  • The repo is an NX monorepo using a custom flat config.
  • Each project contains a tsconfig.json that extends a base config, and references several additional tsconfig files (e.g tsconfig.lib.json/tsconfig.spec.json).
  • Linting is run from the root of the workspace on all projects
  • There is also a root level tsconfig.json (as well as the base config) that includes all files outside of nested projects (which theoretically negates the need for a defaultProject if I understand correctly).
  • The ESLint configuration uses a function to return the config at runtime based on environment variables and dynamic values - this all works as expected when editing existing files.
  • Running ESLint from the command line works as expected.

Troubleshooting

So far I've tried:

  • Using project: true in the parser config - ❌ Doesn't work due to the tsconfig.json files not actually including files themselves - the references include/exclude files for their use-case.
  • Using project: [...array of tsconfig.json files] - ✅ Works, but not ideal due to having to manually add any new files to the config each time - it would also be preferred to migrate to the new suggested method (projectService) which works with the CLI.
  • Defining projectService using the object type from the Blog - ❌ Same error for nested files (and not really required for our project

Repro

I have created a repro that has instructions on how to reproduce the error.

@dbaeumer
Copy link
Member

dbaeumer commented Aug 6, 2024

@tetarchus thanks for the issue and the repro !

I was able to reproduce this but IMO this is something that needs to be addressed in the typescript-eslint module. Using the old way (without a project service) correctly validates new files. I don't know why the new way fails here.

The reason why this works for npm run lint is that here a new project service is created on every lint run. In the VS Code extension I keep the linter alive. Otherwise linting a single file with rules that require type information would take a long time.

Can you please open an issue in the corresponding typescript-eslint repository and CC me on it.

@JoshuaKGoldberg
Copy link
Contributor

@dbaeumer how does this interact with #1774?

We'd talked about doing a similar "watch program" in typescript-eslint/typescript-eslint#9353, but it's quite complex. As I understand it, a big root of the issue is that vscode/vscode-eslint doesn't update ESLint in general when there are relevant file system changes. We'd hacked in watch program logic for parserOptions.project previously, but per #1774 it's buggy and really isn't enough.

@dbaeumer
Copy link
Member

dbaeumer commented Aug 7, 2024

@JoshuaKGoldberg thanks for reaching back.

As I understand it, a big root of the issue is that vscode/vscode-eslint doesn't update ESLint in general when there are relevant file system changes.

The VS Code eslint extension has no idea what relevant code changes are. For me eslint is more or less a black box. To mitigate things I added support to re-evalute files on focus change and added an action to re-validate all files on user request. However in this scenario I would need to drop the ESLint instance and re-create it which when type information is needed is to my knowledge very costly. So I avoid this as much as possible. I have no problem to have special code for typescript-eslint but I would need a sort of callback from the plugin to tell me that I should drop the current instance and create a new one. Could something like this be added?

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Aug 7, 2024

a sort of callback from the plugin to tell me that I should drop the current instance and create a new one

Just confirming: as in, typescript-eslint would notify vscode-eslint that some new file exists on disk and has invalidated the project service? If so: I'm doubtful this would be reasonably doable. I suspect this would require us setting up file watchers & such similar to the existing getWatchProgramsForProjects.ts, which is a lot of extra processing & code-work on the typescript-eslint side. Unless I'm misreading this? cc @bradzacher

Alternate idea: we know that the "was not found by the project service" error only happens in the case of misconfiguration. Maybe typescript-eslint could try reloading projects in that case for editors? Rough pseudocode that doesn't have handling for the npx eslint --fix use case:

let opened = serviceSettings.service.openClientFile(/* ... */);

if (!opened.configFileName && !parseSettings.singleRun) {
  serviceSettings.service.reloadProjects();
  opened = serviceSettings.service.openClientFile(/* ... */);
}

I tried this out locally and it fixed the reproduction case.

drop the ESLint instance and re-create it

Would it be helpful if typescript-eslint exposed an API for consumers such as vscode-eslint to signal they'd like to reload the project service? Rough pseudocode:

let reloadProjectsHook: () => void;

// (deep within vscode-eslint's resolveSettings)
const parserOptions = {
  // (whatever is provided by users)
  projectService: true,

  // (injected by editors)
  registerForProjectServiceReload(reloadProjects) {
    reloadProjectsHook = reloadProjects;
  },
};

// (elsewhere)
function calledWhenFocusChanges() {
  reloadProjectsHook?.();
}

...where internally, the reloadProjects function would call the project service's reloadProjects()?

@bradzacher
Copy link
Contributor

Spitballing - could we setup an env var that vscode sets that we use to decide to allow the project service to attach file watchers on the disk so we don't need to manually reload things?

@dbaeumer
Copy link
Member

dbaeumer commented Aug 9, 2024

@JoshuaKGoldberg if you can detect the case and reload the project service without me reloading eslint that would be perfect. In that case we wouldn't need such a callback API sine the is nothing for me to do.

@bradzacher
Copy link
Contributor

From our point of view it's really hard for us to tell exactly what state the disk is in relation to what state the user's config says we should allow.

We don't have a whole lot of signals from TS.

Restarting the project service is of a very expensive operation because it would have to rebuild the type information from scratch - so we need to avoid this if we can.

The best way to fix this is disk watchers so we can have the service incrementally update itself and avoid a costly restart.

Ultimately though we cannot attach a disk watcher because we have no way to tell if it's safe to attach one. For example if we guess and get it wrong then a CLI run could stay running forever because we wouldn't ever close the disk watcher.

However if we can work together to devise a way to signal to us that we're running in an IDE - then we would know that it's safe to attach disk watchers - allowing us to incrementally update.

Other IDEs could then use what we devise here - meaning we would be working towards creating a standard for all IDEs so that we can fix this for everyone.


In its simplest form I'd think an environment variable would do a job. Eg if we lookout for something like TYPESCRIPT_ESLINT_ALLOW_WATCHERS then you could set that when spawning the ESLint process and we'd be able to do the rest.

What do you think of such an idea?

@dbaeumer
Copy link
Member

dbaeumer commented Aug 9, 2024

If you would only do file watching in the IDE case then we might be able to reuse the file watchers that are already available in most IDEs. VS Code does this for TypeScript now. The flow would be as follows:

  • the eslint extension plugs API into the typescript-eslint plugin that allows to create:
    • a directory watcher
    • a single file watcher
    • dispose a watcher
  • the typescript-eslint plugin would use that API to create watchers
  • the eslint-extension would let the typescript-eslint plugin know about these file changes.

Would this ease things for you? You wouldn't need to handle all the special cases of file watching on different platform :-)

@JoshuaKGoldberg
Copy link
Contributor

Would this ease things for you?

YES! Very much.

If you pass this to ESLint -> typescript-eslint via parserOptions, we can use it. That'd be great.

@bradzacher
Copy link
Contributor

We probably don't need to worry about the watching ourselves - right? Doesn't the project service have all the infra built in by default?


The complicated thing is that passing non-serializable things via eslint configs won't work forever - eg if/when eslint becomes multi-threaded then such values need to pass across threads - so if you're passing callbacks then it'll break things.

If we want to go that route we will need to carefully design the API with that in mind.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Aug 10, 2024

project service have all the infra built in by default?

It does, but:

  • It'd be duplicating what VS Code does
  • parserOptions.project users will still have to go through typescript-eslint's more-flawed watching

passing non-serializable things via eslint configs won't work forever

That's a good point. My hunch is there'd need to be something yucky, like a global variable or global registration from typescript-eslint, used. E.g. require('@typescript-eslint/parser').registerWatcherFactory?.(vscodeEslintWatcherFactory).


Edit: by the way, I filed typescript-eslint/typescript-eslint#9772 on typescript-eslint to try to help with the general case of project services needing to be reloaded. I don't think it's a full solution - we should still discuss this IMO.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Aug 28, 2024

Update: [email protected] and newer generally fix the problem of new files not being picked up by the project service. Up to once every 250ms, if the "Parsing error: [path-to-newly-created-file] was not found by the project service ..." error is seen, the parser will reload the project service. That should generally cause it to find the new file.

However, that's a pretty fragile band-aid fix. It only handles the case of a file not being found, not any cases around the file's information being out-of-date. This issue is still relevant & we'd still be interested in receiving file watcher info on the typescript-eslint side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info-needed Issue requires more information from poster
Projects
None yet
Development

No branches or pull requests

4 participants