search

facebook / lexical

Lexical is an extensible text editor framework that provides excellent reliability, accessibility and performance.
https://lexical.dev
MIT License
17.5k stars 1.45k forks source link

fix: allow tsc to typecheck tests, fix type issues in those tests #5982

Closed etrepum closed 2 weeks ago

etrepum commented 2 weeks ago

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

vercel[bot] commented 2 weeks ago

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
etrepum commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

looks good to merge!

etrepum commented 2 weeks ago

Latest commit just fixes the conflicts with main