Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix(eslint-plugin): [prefer-nullish-coalescing] treat any/unknown as non-nullable#8262

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
JoshuaKGoldberg merged 3 commits intotypescript-eslint:mainfromauvred:fix/8261
Jan 28, 2024

Conversation

auvred
Copy link
Member

@auvredauvred commentedJan 16, 2024•
edited
Loading

PR Checklist

Overview

#8089 introduced change in theisNullableType function. It started treatingany andunknown as nullables, it worked forno-unnecesary-type-assertion, but not forprefer-nullish-coalescing.prefer-nullish-coalescing expectsisNullableType to return true only fornull andundefined

FloEdelmann, epmatsw, kmelancholiy, eugene1g, Zamiell, kachkaev, dkimmich-onventis, Bardiamist, mkosir, and RamonBalthazar reacted with thumbs up emojimkosir, jschill, and RamonBalthazar reacted with heart emojimkosir, jschill, and kachkaev reacted with rocket emoji
@typescript-eslint
Copy link
Contributor

Thanks for the PR,@auvred!

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 onhttps://opencollective.com/typescript-eslint.

@netlifyNetlify
Copy link

netlifybot commentedJan 16, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commitce3d8e0
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/65b1199a8d438a00087a70f7
😎 Deploy Previewhttps://deploy-preview-8262--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: 90 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

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

@Josh-Cena
Copy link
Member

Josh-Cena commentedJan 17, 2024•
edited
Loading

I'm good with this solution 👍 I wonder though, shouldn't we recommend switching to nullish coalescing for any/unknown too, with an option to turn that off, just like primitives? Because any/unknown are also nullable, it doesn't make no sense to use nullish coalescing.

@dacarley
Copy link

Has anyone confirmed that this change fixes#8261 and#8276? If not, I may try to confirm it tomorrow.

If confirmed, can the final question about nullish coalescing be deferred into a new PR?

@@ -309,7 +309,7 @@ export default createRule<Options, MessageIds>({
): void {
const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
const type = checker.getTypeAtLocation(tsNode.left);
if (!isNullableType(type)) {
if (!isTypeFlagSet(type, ts.TypeFlags.Null | ts.TypeFlags.Undefined)) {

Choose a reason for hiding this comment

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

@Josh-Cena threading#8262 (comment):

I'm good with this solution 👍 I wonder though, shouldn't we recommend switching to nullish coalescing for any/unknown too, with an option to turn that off, just like primitives? Because any/unknown are also nullable, it doesn't make no sense to use nullish coalescing.

Hmm, good question. I think if the type is explicitly not known then it'd be risky to suggest making any concrete changes to its handling. I'd be in support of this as an opt-in in for v6 andmaybe an opt-out for v7.

Choose a reason for hiding this comment

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

@dacarley threading#8262 (comment):

Has anyone confirmed that this change fixes#8261 and#8276? If not, I may try to confirm it tomorrow.

If confirmed, can the final question about nullish coalescing be deferred into a new PR?

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Code changes LGTM! Just requesting the tests declare their variables fully. Thanks!

@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelJan 24, 2024
@github-actionsgithub-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party labelJan 24, 2024
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Swell, thanks!

@JoshuaKGoldbergJoshuaKGoldberg merged commit70505e4 intotypescript-eslint:mainJan 28, 2024
@ArnaudBarre
Copy link
Contributor

ArnaudBarre commentedJan 29, 2024•
edited
Loading

I can't comment on the orignal PR but I don't really like the change introduce in#8089

The only reason is that it fixes an issue that I don't understand because any assertion on unkown or any is effectively useless, you can look at the inferred type here:https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBAYwDYEMDOa4EE4G8BQccAJijCgPwBccArgHYDW9EA7vQNz4C+++okWHHQBPegjgAzBghgBLCPTgxgaGAAoAlHkJwkweCho4AvHHrBW2LVyJQDtKEpQA6UuS68gA

Screenshot of a the TS playground linked above with the inferred type being Promise<unkown>

@github-actionsGitHub Actions
Copy link

Uh oh!@ArnaudBarre, the image you shared is missing helpful alt text. Check#8262 (comment).

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text atBasic writing and formatting syntax: images on GitHub Docs.

danvk pushed a commit to danvk/typescript-eslint that referenced this pull requestFeb 4, 2024
…non-nullable (typescript-eslint#8262)* fix(eslint-plugin): [prefer-nullish-coalescing] treat any/unknown as non-nullable* chore: rm unrelated changes* test: add declarations of 'y' variable
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsFeb 6, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: [prefer-nullish-coalescing] <internal error when linting on binary expression (||) with destruct tuple and object with rename property>
5 participants
@auvred@Josh-Cena@dacarley@ArnaudBarre@JoshuaKGoldberg
[8]ページ先頭
©2009-2025 Movatter.jp