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(eslint/no_undef): typescript utility types were detected as undefined variables #7195

Closed
wants to merge 2 commits into from

Conversation

Spoutnik97
Copy link
Contributor

@Spoutnik97 Spoutnik97 commented Nov 7, 2024

First contribution to this repo, and first contribution in Rust ! So, please, help me to improve my skills ;) $

fix #7007, fix #7008

Copy link

graphite-app bot commented Nov 7, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added A-linter Area - Linter C-bug Category - Bug labels Nov 7, 2024
);

const TS_UTILITY_TYPES: [&str; 25] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

While I don't actually know how to implement it, @Boshen mentioned checking for symbol flags in the issue. I assume that would mean not using a hard coded list like this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm yeah this is rough, we would have to keep this list exactly up to date with the globals TS requires.

also, if a user was using a package that declares a global (type or var) this would break

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't found the ts types list in the repo. So this a first proposal to raise this question too :)

@Boshen
Copy link
Member

Boshen commented Nov 9, 2024

Yeah this doesn't work, we need to check for symbols flags.

@Boshen Boshen closed this Nov 9, 2024
@camc314
Copy link
Collaborator

camc314 commented Nov 9, 2024

Yeah this doesn't work, we need to check for symbols flags.
we'd need multi-file analysis for this right?
we'd effectivly have to load in all of node_modules/and search through source for any global decls and get the symbol flags correct?

@Spoutnik97
Copy link
Contributor Author

Yeah this doesn't work, we need to check for symbols flags.

@Boshen How do you recommend me to do that ?

@camc314
Copy link
Collaborator

camc314 commented Nov 11, 2024

yeah not sure there's a trivial fix for this.
i don't think this rule is recommended for TS files really

@Spoutnik97
Copy link
Contributor Author

Yes indeed! Maybe it is not relevant to fix this bug, knowing that if the file is a ts file, then typescript will catch the error before ESLint.

Imo, it is only relevant for JavaScript, isn't it ?

@camc314
Copy link
Collaborator

camc314 commented Nov 11, 2024

yeah probably only relevant for JS files.
this may still produce false positives in JS files (e.g. global defined in one but referenced in another)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter C-bug Category - Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

linter: eslint/no-undef fails on ts type NodeListOf linter: eslint/no-undef does not know about native types
4 participants