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

single-run inference should work with default program creation #3627

Closed
JamesHenry opened this issue Jul 11, 2021 · 11 comments
Closed

single-run inference should work with default program creation #3627

JamesHenry opened this issue Jul 11, 2021 · 11 comments
Labels
enhancement New feature or request package: parser Issues related to @typescript-eslint/parser wontfix This will not be worked on

Comments

@JamesHenry
Copy link
Member

Opening this as a tracker for myself - I realized currently there is no way for single-run inference to be used if the project is also relying on "createDefaultProject".

It will probably require a little bit of refactoring to make this combination possible but hopefully it shouldn't be too bad. I'll hopefully be able to work on it this week.

@JamesHenry JamesHenry added package: parser Issues related to @typescript-eslint/parser triage Waiting for maintainers to take a look labels Jul 11, 2021
@JamesHenry JamesHenry self-assigned this Jul 11, 2021
@JamesHenry JamesHenry removed the triage Waiting for maintainers to take a look label Jul 11, 2021
@bradzacher
Copy link
Member

I don't think we should support this. Ideally I wanted to remove createDefaultProgram in the next major.

This option is only really used by bad configs from what I've seen.

@JamesHenry
Copy link
Member Author

I’m afraid it’s required for the Angular ecosystem right now, the Angular CLI scaffolds projects which do not cover all their files with tsconfigs because they invented a paradigm around environment files being magically swapped as part of the build.

@JamesHenry
Copy link
Member Author

FWIW I believe they regret doing it that way, but AFAIK there is no alternative on the horizon

@JamesHenry
Copy link
Member Author

JamesHenry commented Jul 11, 2021

Just to add more important context, it actually goes further than that magical environment file swapping... They don't use glob patterns for source .ts includes, they use specific file entrypoints:

E.g. tsconfig.app.json

{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "outDir": "./out-tsc/app",
    "types": []
  },
  "files": [
    "src/main.ts",
    "src/polyfills.ts"
  ],
  "include": [
    "src/**/*.d.ts"
  ]
}

...so this also means that that when you create new files as part of normal development they are not covered by the Programs either (until you import the new files into existing ones which are) and you will therefore also get eager red squigglies and errors from typescript-eslint

image

If it were only the environment file swapping we could work around it with ignorePatterns, but this DX above is just awful and so that is why I am forced to leverage createDefaultProgram

@bradzacher
Copy link
Member

Perhaps thats something we should take up with the TS team instead? The watch program builder should be able to update the set of files after a rebuild if you're using the files option.

@JamesHenry
Copy link
Member Author

JamesHenry commented Jul 11, 2021

@bradzacher Sorry maybe I didn't make the example clear enough.

The entrypoints in files are main.ts and polyfills.ts - let's ignore polyfills because it doesn't change.

Let's say our whole app is:

main.ts (bootstraps the Angular app)
-> imports a.ts
-> imports b.ts

main.ts, a.ts, b.ts will all be linted fine because of these relationships starting from main.ts which is in "files"

As part of normal development we want to work on a new component, so we create a new file, c.ts

c.ts will immediately fail linting because it is not imported by main.ts or any of the things it imports yet.

This is not something that relates to watch programs or an issue with TS.

If we then update main.ts OR a.ts OR b.ts to import c.ts linting will then be possible but as I say this is seriously clunky DX and you will always have these errors with new files until you import them into the existing dependency graph of the Program.

Do you see what I mean?

@bradzacher
Copy link
Member

Yeah that makes sense

So why don't we just use a separate tsconfig.eslint.json to specify the files with a glob pattern? Thats what we generally recommend for most people with incompatible setups. Seems like a "best of both worlds" setup then - build using the strict set of files, and lint using the glob pattern.

The lint can even extend the base config to keep everything the same.

@JamesHenry
Copy link
Member Author

I suggested the same thing straight off the bat over a year ago and it was rejected by the Angular Team at the time because they did not want another file...

Now having said that, things have changed since then and the decision was made to not include linting in the Angular CLI out of the box (and therefore the Angular Team will not maintain any linting stuff, it all falls to me), so it may be worth revisiting that constraint...

I have always documented it as an option for users to explore if they are experience performance issues, but I could potentially invert it in the next major version and give people the extra file by default

@JamesHenry
Copy link
Member Author

PS my vote would be in the next major version to rename the option to deprecated__createDefaultProgram and then remove in v6 as most people won't have any sense currently that it is close to being removed. Having that rename will at least give users an immediate impetus to change things up at v5 but a fallback for a while if they can't

@bradzacher
Copy link
Member

I do wonder if we could look to VSCode for inspiration here and move to an automatic hybrid model?
From memory I think that VSCode uses the standard programs built from the tsconfigs where it can, but whenever it encounters a file that doesn't match a program, it will then create a single "catch-all" program and add the file to that program.

We almost do this right now, but I think we create one program PER unknown file, instead of just a single catch-all program.

@bradzacher bradzacher added the enhancement New feature or request label Jul 12, 2021
@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 25, 2021
@JoshuaKGoldberg
Copy link
Member

Per #5857 this is deprecated behavior and we won't be accepting PRs to enhance it. Hooray!

@JoshuaKGoldberg JoshuaKGoldberg added wontfix This will not be worked on and removed accepting prs Go ahead, send a pull request that resolves this issue labels Apr 16, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request package: parser Issues related to @typescript-eslint/parser wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants