Skip to content
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

Merged
merged 2 commits into from
May 2, 2024

Conversation

etrepum
Copy link
Collaborator

@etrepum etrepum commented Apr 28, 2024

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

Copy link

vercel bot commented Apr 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 29, 2024 5:32pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 29, 2024 5:32pm

Copy link
Contributor

@potatowagon potatowagon left a 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';
Copy link
Contributor

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

Suggested change
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';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import {createRoot, Root} from 'react-dom/client';
import type {Root} from 'react-dom/client';
import {createRoot} from 'react-dom/client';

Comment on lines +25 to 26
LexicalEditor,
} from 'lexical';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
LexicalEditor,
} from 'lexical';
} from 'lexical';
import type {LexicalEditor} from 'lexical';

DecoratorNode,
ElementNode,
LexicalEditor,
LexicalNode,
Copy link
Contributor

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
Copy link
Contributor

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

Comment on lines +15 to 17
LexicalEditor,
PointType,
} from 'lexical';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
LexicalEditor,
PointType,
} from 'lexical';
} from 'lexical';
import type {LexicalEditor, PointType} from 'lexical';

potatowagon
potatowagon previously approved these changes Apr 29, 2024
@etrepum
Copy link
Collaborator Author

etrepum commented Apr 29, 2024

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.

@etrepum
Copy link
Collaborator Author

etrepum commented Apr 29, 2024

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

@potatowagon
Copy link
Contributor

potatowagon commented Apr 29, 2024

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!

@potatowagon
Copy link
Contributor

potatowagon commented Apr 29, 2024

looks good to merge!

StyleT
StyleT previously approved these changes Apr 29, 2024
@etrepum etrepum dismissed stale reviews from StyleT and potatowagon via e7aac52 April 29, 2024 17:30
@etrepum
Copy link
Collaborator Author

etrepum commented Apr 29, 2024

Latest commit just fixes the conflicts with main

@StyleT StyleT merged commit a0bb9b0 into facebook:main May 2, 2024
47 checks passed
@etrepum etrepum deleted the fix-test-types branch May 11, 2024 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants