-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Set eqeqeq rule to always and clarify type checks #5941
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
9481930
to
de34549
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Is this still relevant?
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
eqeqeq
eslint rule toalways
, which forces===
,!==
instead of==
,!=
.invariant
.Further steps
undefined
andnull
where 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.->--noUncheckedIndexedAccess
introduced 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.