Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
chore(website): fixed most no-unnecessary-condition complaints#7836
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
chore(website): fixed most no-unnecessary-condition complaints#7836
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@JoshuaKGoldberg! 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. |
netlifybot commentedOct 25, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
nx-cloudbot commentedOct 25, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
| @@ -267,7 +267,7 @@ export const LoadedEditor: React.FC<LoadedEditorProps> = ({ | |||
| return debounce(() => editor.layout(), 1); | |||
| }, [editor]); | |||
| const container = editor.getContainerDomNode?.() ?? editor.getDomNode(); | |||
| const container = editor.getContainerDomNode(); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is not correct, as this part of code is neccesary for some older versions of playground (older version of ts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🤔 I'm confused, where are those types defined? Theeditor.api.d.ts I see hasgetContainerDomNode(): HTMLElement; - and I thought we're settled on just one editor version?
| @@ -50,7 +50,7 @@ export function getRuleJsonSchemaWithErrorLevel( | |||
| }; | |||
| } | |||
| // example: naming-convention rule | |||
| if (typeof ruleSchema.items === 'object' && ruleSchema.items) { | |||
| if (typeof ruleSchema.items === 'object') { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
typeofnull==='object'// true
bradzacherNov 11, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think in the context of the types it's probably defined asitems?: T[] - hence thetypeof is enough to refine the types correctly?
This was a strictness change we added in v6.
We could probably even change this check to be "cleaner" but it'll do fine like this I think
JoshuaKGoldbergNov 13, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah I don't see anynull values foritems in any ESLint or typescript-eslint rule. Is there a reason to support it?
| @@ -67,7 +67,7 @@ export function TypeInfo({ | |||
| onHoverNode, | |||
| }: TypeInfoProps): React.JSX.Element { | |||
| const computed = useMemo(() => { | |||
| if (!typeChecker || !value) { | |||
| if (!typeChecker) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is not correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@armano2 according to the types it is - the propertyvalue is defined asreadonly value: Node and it's never reassigned in the component body
| @@ -48,15 +48,13 @@ export function TypesDetails({ | |||
| value={value} | |||
| /> | |||
| </div> | |||
| {selectedNode && ( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
selectedNode may be undefined
bradzacherNov 11, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@armano2 based on the types it can't be.
the initial value passed touseState is typed asreadonly value: Node.
Then the only call tosetSelectedNode is guarded byif (item.node) { setSelectedNode(item.node) }
unless, of course,SimplifiedTreeView can make itundefined? But based on its types it can't do that I believe (or else there would be a type error on itsonSelect prop with this change)
PR Checklist
Overview
Makes progress on fixing existing
@typescript-eslint/no-unnecessary-conditionviolations underpackages/website. Part of a series of PRs that split this up, as there are many of them.