[lexical][lexical-selection][lexical-utils] Refactor: Consolidate ancestor lookup via findMatchingParent #7814
Conversation
…ils in place of duplicate implementations.\n- Replace local helper in lexical-link and wrap in lexical-selection to delegate to shared utility.\n- Reduces duplicate logic and aligns with guidance in facebook#5311.\n\nPartial fix for facebook#5311
cnaples79
requested review from
Fetz,
Sahejkm,
acywatson,
fantactuka,
ivailop7,
potatowagon,
takuyakanbr and
zurfyx
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Hi @cnaples79! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
meta-cla
bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
| INTERNAL_$isBlock, | ||
| } from 'lexical'; | ||
| import invariant from 'shared/invariant'; | ||
| import { $findMatchingParent } from '@lexical/utils'; |
There was a problem hiding this comment.
@lexical/selection can't depend on @lexical/utils
|
The follow-up work, or equivalent, is necessary for this to work at all because |
|
@etrepum I see! Thanks for the feedback, I'll update the fix and PR. |
|
Thanks for the pointer on package layering, @etrepum. I’ve updated the PR to avoid the circular dependency by moving into core (), exporting it via , and re-exporting from .\n\nChanges: \n- Add to with the same typed overloads and docs.\n- Export from .\n- Update to import from (not ).\n- Re-export from and remove the duplicate local implementation.\n\nThis keeps free of a dependency on , removes the duplication, and aligns with the guidance from #5311. Happy to iterate further if you'd prefer a different placement or naming. |
|
Pushed a follow-up to fix the Vercel failure: removed the duplicate const and added proper overloads to the existing function in (single implementation). This should unblock the playground build. If there’s a preferred location or naming for the overloads/docs, happy to tweak. |
|
Found and fixed the Vercel build failure! 🎉 Issue: There were duplicate function declarations causing TypeScript compilation errors:
Fix: Removed the The TypeScript pattern should be:
This should resolve the build failure. The playground build was already working, so this was specifically affecting the main lexical package build. |
packages/lexical-link/src/index.ts
Outdated
| } | ||
| return predicate(parent) ? parent : null; | ||
| } | ||
| // Removed duplicate $getAncestor in favor of shared $findMatchingParent from @lexical/utils |
There was a problem hiding this comment.
No need for this comment, only really makes sense in the context of the PR, not the remaining code
| // Removed duplicate $getAncestor in favor of shared $findMatchingParent from @lexical/utils |
| } | ||
| return predicate(parent) ? parent : null; | ||
| // Delegate to shared implementation to avoid duplication. | ||
| return $findMatchingParent(node, predicate) as NodeType | null; |
There was a problem hiding this comment.
This doesn't remove $getAncestor or fix the type of $findMatchingParent per #5311 (comment)
packages/lexical-utils/src/index.ts
Outdated
|
|
||
| return null; | ||
| }; | ||
| export { $findMatchingParent } from 'lexical'; |
There was a problem hiding this comment.
This doesn't work, it's used in this module so you need to import it then separately export it.
Are you running this code to test it? Pretty sure npm run ci-check would've caught this pretty quickly
packages/lexical/src/LexicalUtils.ts
Outdated
| while (curr !== $getRoot() && curr != null) { | ||
| if (findFn(curr)) { | ||
| return curr; | ||
| return curr as LexicalNode; // T is inferred via overload when appropriate |
There was a problem hiding this comment.
This cast seems redundant, and all casts are potentially unsafe so you should avoid them unless absolutely necessary
- Remove duplicate $findMatchingParent export from lexical-utils (keep only bulk re-export) - Remove duplicate local $getAncestor function from lexical-selection (keep only exported one) - Fix all linting and formatting issues - TypeScript compilation now passes without errors
cnaples79
force-pushed
the
refactor-findMatchingParent-unify
branch
from
f22a1c8 to
0b2bf11
Compare
… $findMatchingParent - Remove $getAncestor function definition from lexical-selection completely - Replace all $getAncestor calls with $findMatchingParent direct calls - Remove PR-context comment about 'Delegate to shared implementation' - This provides full replacement rather than delegation, as requested by @etrepum Addresses: facebook#7814 (comment)
|
I've updated the PR and addressed your feedback @etrepum! |
etrepum
left a comment
etrepum
left a comment
There was a problem hiding this comment.
Made some suggestions for improvements to the existing code. Audited our existing usage of $getAncestor (not publicly exported, and does not depend on the implementation differences with $findMatchingParent, so it's fine to remove).
etrepum
added
extended-tests
|
This looks good, other than the PR title and description. Can you change these to match the repository's pull request template? It's important for how we put together the release notes. https://raw.githubusercontent.com/facebook/lexical/refs/heads/main/.github/pull_request_template.md |
cnaples79
changed the title
cnaples79
changed the title
|
@etrepum done! Please let me know if any other changes to the PR need to be made! And thank you again for your feedback. |
2 participants
Description
Current behavior:
$findMatchingParentand$getAncestorlogic lived in multiple places (@lexical/utils,@lexical/selection), leading to duplicated implementations and layering violations. This duplication also surfaced as duplicate symbol/implementation errors in CI when both versions were in play.Changes in this PR:
$findMatchingParentinto core (packages/lexical/src/LexicalUtils.ts) with overloads (type‑predicate support).$findMatchingParentfrom core (packages/lexical/src/index.ts).@lexical/utils: re‑export$findMatchingParentfromlexicaland remove the duplicate implementation.@lexical/selection: provide a local exported$getAncestorwrapper that delegates to core$findMatchingParent(import fromlexical), avoiding a utils dependency and removing local duplication.@lexical/link(no functional change).No runtime behavior changes are intended; this is a consolidation/refactor to respect package layering and eliminate duplicate code.
Closes #5311
Test plan
Before
$findMatchingParentwhen both versions compiled.After
No visual changes.