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] Feature: error checking for node key re-use with type mismatch in __DEV__ #6014

Merged
merged 1 commit into from May 11, 2024

Conversation

etrepum
Copy link
Contributor

@etrepum etrepum commented May 2, 2024

Description

Based on issues that several people have run into, it seems useful to add some additional invariant checking around node keys.

See also: discord message

  • Adds a stub implementation of shared/invariant for use under jest to make development easier
  • Adds a __DEV__ check to make sure that if a new Node is created with an explicit existing key, that the type of the node did not change

Test plan

Before

Bad things will silently happen if you accidentally re-use node keys, such as with a subtly incorrect node replacement function:

const config = {
  // ...
  nodes: [
    EdifactTextNode,
    {
      replace: TextNode,
      with: (node) => {
        return new EdifactTextNode(node.text, node.__key)
      },
    },
  ],
}

After

An error message is shows up now, in __DEV__. There are unit tests that verify that the specific error message comes up when creating a new node in this way (tested in isolation and with a node replacer).

Copy link

vercel bot commented May 2, 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 May 3, 2024 3:46pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 3, 2024 3:46pm

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.

looks good 👍

@acywatson
Copy link
Contributor

I have a small concern about the size here - would be nice to be able to tree-shake this stuff out of the prod build.

@acywatson
Copy link
Contributor

I have a small concern about the size here - would be nice to be able to tree-shake this stuff out of the prod build.

Discussed offline - we'll follow up and ensure that we're doing what we can to shake out these DEV things in rollup

@acywatson acywatson merged commit 9db4cad into facebook:main May 11, 2024
46 checks passed
@etrepum
Copy link
Contributor Author

etrepum commented May 11, 2024

Just for posterity's sake I did confirm that unused functions that do not get exported are tree-shaken correctly in both the .mjs and .js prod builds at this time. Had this functionality not worked, it would also show up elsewhere (e.g. handleDEVOnlyPendingUpdateGuarantees is only used under __DEV__ in LexicalUpdates.ts)

@etrepum etrepum deleted the dev-key-constructor-check branch May 11, 2024 18:59
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.

None yet

4 participants