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

LexicalBlockNormalizer #6025

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

LexicalBlockNormalizer #6025

wants to merge 3 commits into from

Conversation

zurfyx
Copy link
Member

@zurfyx zurfyx commented May 4, 2024

LexicalBlockNormalizer (aka NestingEnforcementPlugin)

This was probably our first ever formal normalizer after some patches in the HTML importDOM functions (it has been internal only for a while). The idea behind this normalizer is to make sure that non-inline elements are always at the very top of the tree.

Invalid states can happen anytime and there is no mechanism to enforce them. The idea behind Schemas #3833 is still not very appealing for the fact that we trade-off state consistency for reliability which, from a user perspective, is still bad reliablity. The idea of normalization seems to have gotten more traction over the past years, when we opened an API to do static transforms and started standalone normalizers (i.e. table #5824). FWIW, I'm not a fan of static transforms but the point still stands.

While this plugin won't be the final shape of normalizers, this brings us a step closer to having a formal API to define plugins, which can standardize normalizers.

Credit for the implementation goes to @fantactuka, I just ported the code and adjusted the API.

Note for reviewers: the best test plan I can offer for this is the unit test itself. The 11 test cases will walk you through what this plugin can normalize.

@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 May 4, 2024
Copy link

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

Copy link

github-actions bot commented May 4, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 23.7 KB (0%) 475 ms (0%) 708 ms (+56.52% 🔺) 1.2 s
packages/lexical-rich-text/dist/LexicalRichText.js 34.35 KB (0%) 687 ms (0%) 1.7 s (+4.93% 🔺) 2.4 s
packages/lexical-plain-text/dist/LexicalPlainText.js 34.29 KB (0%) 686 ms (0%) 1.4 s (+2.66% 🔺) 2.1 s

ivailop7
ivailop7 previously approved these changes May 4, 2024
Copy link
Collaborator

@ivailop7 ivailop7 left a comment

Choose a reason for hiding this comment

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

The failures might be related to the Rollup/Vite merge. LGTM otherwise. Thanks for this!

@zurfyx
Copy link
Member Author

zurfyx commented May 4, 2024

Hey @etrepum, do you have any idea what's causing the integrity issues above? I added a new file inside @lexical/react and run npm run update-packages. Admittedly, it's quite a time-saver, good job on that one! but I wonder why it's now throwing errors for completely unrelated files here.

##[debug]Dropping file value '/home/runner/work/node_modules/@types/babel__core/index.d.ts'. Path does not exist
Error: ../../node_modules/@types/babel__core/index.d.ts(2,31): error TS2792: Cannot find module '@babel/parser'. Did you mean to set the 'moduleResolution' option to 'nodenext', or to add aliases to the 'paths' option?
##[debug]Dropping file value '/home/runner/work/node_modules/@types/babel__core/index.d.ts'. Path does not exist
Error: ../../node_modules/@types/babel__core/index.d.ts(5,20): error TS2792: Cannot find module '@babel/types'. Did you mean to set the 'moduleResolution' option to 'nodenext', or to add aliases to the 'paths' option?

@etrepum
Copy link
Collaborator

etrepum commented May 4, 2024

@zurfyx in my experience those sort of errors are caused by a bad cache, maybe we have something that is populating the cache incorrectly somewhere? I think maybe the solution to this is to change our workflows to use explicit save and restore points so we don't end up populating the cache with weird stuff. I'll do a PR for that today.

return;
}

invariant(
Copy link
Contributor

Choose a reason for hiding this comment

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

Sort of minor, but do we need to run this check on every transform? can we just check in the render function? Maybe in an effect?

@ivailop7
Copy link
Collaborator

ivailop7 commented May 5, 2024

@zurfyx in my experience those sort of errors are caused by a bad cache, maybe we have something that is populating the cache incorrectly somewhere? I think maybe the solution to this is to change our workflows to use explicit save and restore points so we don't end up populating the cache with weird stuff. I'll do a PR for that today.

I thought I replied, but either I didn't submit or I commented on the wrong PR. Even after the merged CI fix, I reran the tests here and they still fail. 😕

const emptyFunction = () => {};

/**
* Ensures that block nodes live at the very top of the tree.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need much better documentation here, especially now that we're getting more caught up on docs and organization.

I understand what this does, but I don't think we've clearly communicated who should use it and when, what practical problems it solves, etc.

I think one thing we need at least, in addition to this, is a list of constraints or rules that we plan to enforce, like Slate:

https://docs.slatejs.org/concepts/11-normalizing#built-in-constraints

We actually share a lot of these, but I don't think a magic plugin is a great solution long-term without a clear description of what it's doing and why.

@acywatson
Copy link
Contributor

FWIW, I'm not a fan of static transforms but the point still stands.

I don't think you've ever adequately explained your objection here - we should discuss this more, as there's a fundamental directional choice here between node-centric APIs, editor-centric APIs, and a combination of both.

@etrepum
Copy link
Collaborator

etrepum commented May 5, 2024

I thought I replied, but either I didn't submit or I commented on the wrong PR. Even after the merged CI fix, I reran the tests here and they still fail. 😕

@ivailop7 The CI fix was merged to main, but main hasn't been merged into this PR

@ivailop7
Copy link
Collaborator

ivailop7 commented May 5, 2024

I thought I replied, but either I didn't submit or I commented on the wrong PR. Even after the merged CI fix, I reran the tests here and they still fail. 😕

@ivailop7 The CI fix was merged to main, but main hasn't been merged into this PR

You are right, my fault.

@etrepum
Copy link
Collaborator

etrepum commented May 5, 2024

@acywatson I personally think the static node-centric transform (or some new API that like a transform) makes sense in cases where you need to enforce some ElementNode invariants or constraints related to children or parents when a node is dirty. The other benefit is that you get this normalization in subclasses whereas otherwise you have some work to do (and maybe you can't actually do it without a mess of copy paste because the plugin didn't export the transform function for you) if you extend a node that has these kinds of constraints.

@acywatson
Copy link
Contributor

@acywatson I personally think the static node-centric transform (or some new API that like a transform) makes sense in cases where you need to enforce some ElementNode invariants or constraints related to children or parents when a node is dirty. The other benefit is that you get this normalization in subclasses whereas otherwise you have some work to do (and maybe you can't actually do it without a mess of copy paste because the plugin didn't export the transform function for you) if you extend a node that has these kinds of constraints.

Yea, I'm inclined to agree - recalling past conversations, I think the principled basis for Gerard's disagreement with this approach is that we should somehow limit the number of ways that one can do something in Lexical, in order to just simplify the developer experience.

I personally think that there seem to be use cases where each of these different approaches is more ergonomic, and providing the flexibility in those cases outweighs any confusion that might arise from having two ways to declare a transform. There's a similar thing with importDOM/exportDOM - the node-centric approach is pretty great, but makes certain common cases really awkward (like overriding text node import/export to add more styles).

@etrepum
Copy link
Collaborator

etrepum commented May 5, 2024

It's definitely worth considering if there's a more general, but still accessible, way to achieve these sorts of features. Possibly something worth considering would be to have a more general static method when a node is attached to an editor and get rid of the static transform, e.g.:

class WhoNeedsAPluginNode extends LexicalNode {
  static registerWithEditor(editor: LexicalEditor): void {
    // I don't think it makes any sense to return the dispose functions b/c you can't unregister a node
    editor.registerNodeTransform(this, doStaticTransform);
    editor.registerCommand();
  }
}
function createEditor() {
  const editor = new LexicalEditor();
  for (const klass of registeredNodes.values()) {
    if (klass.registerWithEditor !== LexicalNode.registerWithEditor) {
      klass.registerWithEditor(editor);
    }
  }
}

I think it would be fine as long as this was sequenced after the editor was created and before the non-empty initialState is attached?

@ivailop7
Copy link
Collaborator

ivailop7 commented May 7, 2024

This is become a larger discussion that is larger than this PR, however, let's go ahead with this for now. Since it's proven and it works. @zurfyx can we go forward with this one.

@GermanJablo
Copy link
Contributor

I copy what I said in #6066:

I do not understand what problem normalizers solve that do not already solve transforms as has been exemplified in the last comments of #3833. Maybe I'm missing something, but if not, I'm leaning toward not adding a new API.

The only difference I see between this PR and the SchemaPlugin that were shown in the last 3 comments of #3833 is that a generic normalization can be offered for all blocks, regardless of which and how many they are, and whether the user knows about them or no.

I'm not sure if I like that something so important and opinionated is done implicitly by the library. I think a SchemaPlugin or NormalizedPlugin in Getting Started of the documentation would give more power and flexibility to users, and would not add another registrable API to the editor, which already has quite a few.

return;
}

const event = lastPasteCommand + 250 > Date.now() ? 'paste' : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the 250 for? isit 250 ms?

@ivailop7
Copy link
Collaborator

FYI ported this over internally to test and it broke the collapsible sections. Haven't investigated why yet.

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.

7 participants