-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: allow tsc to typecheck tests, fix type issues in those tests #5982
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 only non test file modified is
packages/lexical-selection/src/__tests__/utils/index.ts
. adds typing only so low risk change - rest of code changes to test files and adds typing -> very low risk change
- left some minor nits about importing as type
looks good! thankyou for this :)
@@ -34,12 +34,12 @@ import { | |||
} from 'lexical/src'; | |||
import {createTestEditor, TestComposer} from 'lexical/src/__tests__/utils'; | |||
import React from 'react'; | |||
import {createRoot} from 'react-dom/client'; | |||
import {createRoot, Root} from 'react-dom/client'; |
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.
nit: we could import Root as type instead
import {createRoot, Root} from 'react-dom/client'; | |
import type {Root} from 'react-dom/client'; | |
import {createRoot} from 'react-dom/client'; |
@@ -8,22 +8,22 @@ | |||
|
|||
import {$createTextNode, $getRoot, ParagraphNode, TextNode} from 'lexical'; | |||
|
|||
import {createTestConnection, waitForReact} from './utils'; | |||
import {Client, createTestConnection, waitForReact} from './utils'; |
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.
import {Client, createTestConnection, waitForReact} from './utils'; | |
import type {Client} from './utils'; | |
import {createTestConnection, waitForReact} from './utils'; |
@@ -8,14 +8,14 @@ | |||
|
|||
import {useLexicalComposerContext} from '@lexical/react/LexicalComposerContext'; | |||
import * as React from 'react'; | |||
import {createRoot} from 'react-dom/client'; | |||
import {createRoot, Root} from 'react-dom/client'; |
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.
import {createRoot, Root} from 'react-dom/client'; | |
import type {Root} from 'react-dom/client'; | |
import {createRoot} from 'react-dom/client'; |
} from 'lexical'; | ||
import * as React from 'react'; | ||
import {createRoot} from 'react-dom/client'; | ||
import {createRoot, Root} from 'react-dom/client'; |
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.
import {createRoot, Root} from 'react-dom/client'; | |
import type {Root} from 'react-dom/client'; | |
import {createRoot} from 'react-dom/client'; |
LexicalEditor, | ||
} from 'lexical'; |
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.
LexicalEditor, | |
} from 'lexical'; | |
} from 'lexical'; | |
import type {LexicalEditor} from 'lexical'; |
DecoratorNode, | ||
ElementNode, | ||
LexicalEditor, | ||
LexicalNode, |
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.
ditto nit: could be imported as type instead
3. `editor.update()` schedules a reconciliation microtask and returns | ||
4. `await` schedules a resume microtask and yields control to the task executor | ||
5. the reconciliation microtask runs, reconciling the editor state with the DOM | ||
6. the resume microtask runs |
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.
👍
thanks for adding this! will leave to @zurfyx and other reconciler experts to vet
LexicalEditor, | ||
PointType, | ||
} from 'lexical'; |
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.
LexicalEditor, | |
PointType, | |
} from 'lexical'; | |
} from 'lexical'; | |
import type {LexicalEditor, PointType} from 'lexical'; |
I'd be happy to fix up those imports if you want, I was just doing whatever the language server suggested since I was fixing a few hundred type errors! Perhaps it would be better to do that cleanup separately in a more systematic way? There are other bad import patterns that we could get rid of in the tests (e.g. importing public code from 'lexical/src' instead of 'lexical', and we could get rid of those tsconfig hacks too). It seems likely that we are also importing values as types elsewhere in there… if that's even something we care about? Type imports are usually done as a performance thing or to prevent import side-effects or circular dependencies if values from the module are not otherwise imported. |
If we do clean up the type-only imports there are linters and typescript flags that should be configured to enforce whatever style is chosen, e.g. https://typescript-eslint.io/rules/consistent-type-imports/ and https://www.typescriptlang.org/tsconfig/#verbatimModuleSyntax are relevant for that |
yes addressing the type imports in a seperate PR makes sense would be great if a lint could autofix this would make more sense to address it as part of an autofix! |
looks good to merge! |
Latest commit just fixes the conflicts with main |
Mostly just adding non-null assertions and adding types where inference fails. I did refactor a few list reduces that do a concat (which is also quadratic time) to use flatMap which is easier for TypeScript to infer.
I added some docs to the FAQ relevant to reconciliation and how the tests are written https://lexical-git-fork-etrepum-fix-test-types-fbopensource.vercel.app/docs/faq#when-does-reconciliation-happen