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

[lexical-mark] Bug Fix: reverse ternary in MarkNode.addID #7020

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

james-atticus
Copy link
Contributor

@james-atticus james-atticus commented Jan 7, 2025

Description

Ternary logic introduced in 7c21d4f#diff-b30867251dac89bfc41aba98899f9ca7e6b1045cc8bcfbcc9e9824caaf53d695R112-R122 is the wrong way around. Trying to add a new ID is a no-op, and adding an existing ID duplicates it.

Test plan

I have the following test code to replace IDs:

console.dir(oldIdToNewId);
console.dir(node.getIDs());

const newId = oldIdToNewId[id];
node.addID(newId);
node.deleteID(id);

console.log('replaced %s with %s', id, newId);
console.dir(node.getIDs());

Which produces the following on 0.23.0:

{
  'e338a2b4-3a70-4a73-9871-6b61f2ed8caa': '57404623-3ae5-5cfa-812f-762e507a69ae'
}
[ 'e338a2b4-3a70-4a73-9871-6b61f2ed8caa' ]
replaced e338a2b4-3a70-4a73-9871-6b61f2ed8caa with 57404623-3ae5-5cfa-812f-762e507a69ae
[]

That last line should be [ '57404623-3ae5-5cfa-812f-762e507a69ae' ]. To further confirm, I changed the code to just call node.addID(id) (i.e. the old ID), which then prints:

[
  'e338a2b4-3a70-4a73-9871-6b61f2ed8caa',
  'e338a2b4-3a70-4a73-9871-6b61f2ed8caa'
]

Copy link

vercel bot commented Jan 7, 2025

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 Jan 7, 2025 4:33am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 7, 2025 4:33am

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 7, 2025
Copy link

github-actions bot commented Jan 7, 2025

size-limit report 📦

Path Size
lexical - cjs 31.39 KB (0%)
lexical - esm 31.19 KB (0%)
@lexical/rich-text - cjs 40.47 KB (0%)
@lexical/rich-text - esm 33.12 KB (0%)
@lexical/plain-text - cjs 39.04 KB (0%)
@lexical/plain-text - esm 30.37 KB (0%)
@lexical/react - cjs 42.3 KB (0%)
@lexical/react - esm 34.42 KB (0%)

Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

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

A test that covers this would be even better, to prevent the next time a refactor causes a regression.

@etrepum etrepum added this pull request to the merge queue Jan 7, 2025
Merged via the queue into facebook:main with commit 7de86e4 Jan 7, 2025
41 checks passed
@james-atticus james-atticus deleted the patch-1 branch January 7, 2025 22:49
@etrepum etrepum mentioned this pull request Jan 9, 2025
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.

3 participants