Set eqeqeq rule to always and clarify type checks #5941
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
2wheeh
changed the title
2wheeh
force-pushed
the
improve-null-check
branch
from
9481930
to
de34549
Compare
2wheeh
changed the title
2wheeh
requested review from
zurfyx,
fantactuka,
acywatson and
Fetz
as code owners
| fullMatchRegExpByTag: Readonly<Record<string, RegExp>>; | ||
| fullMatchRegExpByTag: Readonly<Partial<Record<string, RegExp>>>; | ||
| openTagsRegExp: RegExp; | ||
| transformersByTag: Readonly<Record<string, TextFormatTransformer>>; | ||
| transformersByTag: Readonly<Partial<Record<string, TextFormatTransformer>>>; |
There was a problem hiding this comment.
When the key is not a valid one, it should infer the value as undefined.
Partial wrapping Record enables it.
For instance:
const transformer = textFormatTransformersIndex.transformersByTag[match[1]];
if (transformer) {
for (const format of transformer.format) {
if (!currentNode.hasFormat(format)) {
currentNode.toggleFormat(format);
}
}
}transformer should be inferred as undefined when match[1] is not a valid key.
Otherwise, there is a potential risk of removing if(transformer) check.
| if (childDOM) { | ||
| resolvedNode = getNodeFromDOM(childDOM); | ||
| } |
There was a problem hiding this comment.
let childDOM = childNodes[resolvedOffset];When resolvedOffset is not a valid index, childDOM is assigned as undefined.
This check was missed since TS infers it as ChildNode.
Getting value from array or object (like this and splitText) might need safer way in the end.
| while (currentNode != null) { | ||
| // @ts-expect-error: internal field | ||
| const editor: LexicalEditor = currentNode.__lexicalEditor; | ||
| if (editor != null) { | ||
| while (currentNode !== null) { | ||
| const editor: LexicalEditor | undefined | null = | ||
| // @ts-expect-error: internal field | ||
| currentNode.__lexicalEditor; | ||
| if (editor !== undefined && editor !== null) { |
There was a problem hiding this comment.
The way we handles __lexical** properties are not consistent. It's because we don't use the TS support.
Eventually, we need to eliminating @ts-expect-error when it's possible.
| !$isLineBreakNode(lastNode), | ||
| 'Unexpected lineBreakNode in getEndOfCodeInLine', | ||
| ); | ||
| if ($isLineBreakNode(lastNode)) { |
There was a problem hiding this comment.
Is this an improvement?
Why not leverage the invariant call itself here? Why a separate conditional?
| @@ -349,7 +349,7 @@ function printTargetProperties(node: LinkNode) { | |||
| function printRelProperties(node: LinkNode) { | |||
| let str = node.getRel(); | |||
| // TODO Fix nullish on LinkNode | |||
| @@ -50,7 +50,7 @@ export class InjectedPegasusService | |||
| isReadonly: boolean, | |||
| ): void { | |||
| const editorNode = queryLexicalNodeByKey(editorKey); | |||
| if (editorNode == null) { | |||
| if (editorNode === undefined) { | |||
There was a problem hiding this comment.
I think ideally queryLexicalNodeByKey would return null and never undefined. We try to avoid dealing with undefined anywhere within the library. I don't want to expand the scope of this PR, but maybe a comment.
| 'Expected parentElement of Text not to be null', | ||
| ); | ||
|
|
||
| if (parentDom === null) { |
There was a problem hiding this comment.
Same thing here - it seems inconsistent with how this was done everywhere else.
|
Feel free to reopen if we can get conflicts resolved. |
I tried to improve readability and type safety.
Changes
eqeqeqeslint rule toalways, which forces===,!==instead of==,!=.invariant.Further steps
undefinedandnullwhere one of both would suffice.Developing a general method to infer correct types from arrays or objects considering when index or key is not valid.->--noUncheckedIndexedAccessintroduced in TS4.1 is for this.__lexical**fields on the global namespace (mostly with@ts-expect-error: internal field), similar to the way in devtools.