microsoft / vscode-eslint

VSCode extension to integrate eslint into VSCode
MIT License
1.76k stars 336 forks source link

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

Open tetarchus opened 3 months ago

tetarchus commented 3 months ago

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

Troubleshooting

So far I've tried:

Repro

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

dbaeumer commented 3 months ago

@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 commented 3 months ago

@dbaeumer how does this interact with #1774?

We'd talked about doing a similar "watch program" in https://github.com/typescript-eslint/typescript-eslint/pull/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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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:

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

JoshuaKGoldberg commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

project service have all the infra built in by default?

It does, but:

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 https://github.com/typescript-eslint/typescript-eslint/issues/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 commented 2 months ago

Update: typescript-eslint@8.3.0 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.

mkeyy0 commented 1 month ago

Looks like the issue persists even in the 8.7.0 version. I use the monorepo setup with pnpm and projectService with following configuration:

      files: [VUE_GLOBS],
      languageOptions: {
        parserOptions: {
          parser: options.tsParser,
          extraFileExtensions: ['.vue'],
          sourceType: 'module',
          projectService: {
            allowDefaultProject: ['./*.js'],
            defaultProject: './tsconfig.json',
          },
          tsconfigRootDir: process.cwd(),
        },
      },

Only Restart Extensions Host helps