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

[WIP] Feature: $config protocol + NodeState registration/flattening #7258

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

etrepum
Copy link
Collaborator

@etrepum etrepum commented Feb 27, 2025

Description

Based on the future needs of NodeState (#7117) - specifically allowing nodes to statically declare their required state - this RFC refactors LexicalNode and the associated internals to remove the need for any static methods, vastly reducing the boilerplate in order to subclass nodes. This is a squashed/rebased version of the original #7189 PR that explored this feature.

The short story is that this sort of thing will let us get better type-level information about nodes. We can't have node.getType() ever give a more specific type than string but we could write a $getType(node) that does infer to the exact type if we want. Same thing for node.exportJSON() and similar methods.

It also facilitates scrapping all of the node subclass boilerplate, except for this one method, so long as the class has a compatible signature (trivial constructor)

Example of extension and inference capability from the tests
const numberState = createState('numberState', {
  parse: (v) => (typeof v === 'number' ? v : 0),
});
const boolState = createState('boolState', {parse: Boolean});
class StateNode extends TestNode {
  $config() {
    return this.config('state', {
      extends: TestNode,
      stateConfigs: [{flat: true, stateConfig: numberState}, boolState],
    });
  }
  getNumber() {
    return $getState(this, numberState);
  }
  setNumber(valueOrUpdater: StateValueOrUpdater<typeof numberState>): this {
    return $setState(this, numberState, valueOrUpdater);
  }
}

const extraState = createState('extra', {parse: String});
class ExtraStateNode extends StateNode {
  $config() {
    return this.config('extra-state', {
      extends: StateNode,
      stateConfigs: [{flat: true, stateConfig: extraState}],
    });
  }
}

type _TestExtraStateNodeExportJSON = Expect<
  Equal<
    ExtraStateNodeExportJSON,
    {
      state?:
        | (Record<string, unknown> & {
            boolState?: boolean | undefined;
          })
        | undefined;
      version: number;
      type: 'extra-state';
      numberState?: number | undefined;
      extra?: string | undefined;
    }
  >
>;

Closes #7260

How it works

The proposed protocol for declaring nodes would scale down to something like this:

class CustomTextNode extends TextNode {
  $config() {
    return this.config('custom-text', {extends: TextNode});
  }
}

The two method interface is useful for TypeScript reasons, basically. The outer $config is the API and the this.config(…) is a helper method that has the right generics to make it easy to return something with the right shape and type. The types are very delicate, if you don't have all of the annotations just right then the type will simplify into something without information that can be extracted. Basically it is just returning an object like this, but in a way where TypeScript doesn't collapse the type into something simpler (kind of like using as const or satisfies in the right place):

{'custom-text': {type: 'custom-text', extends: TextNode}}

On the LexicalEditor side, during createEditor, CustomTextNode.prototype.$config() is called and can grab the type and any other useful metadata (e.g. a defined transform, registered state, anything else we want to put in there). For compatibility reasons it will also monkeypatch getType, importJSON, and clone static methods onto the node class if they are not already there. It adds a module global variable to inject cloned node keys so it doesn't need to break any compatibility with multi-argument constructors, it only requires that they have a 0-argument form.

I've also put a $create method in there which reduces the boilerplate and increases the efficiency there, assuming that we'd be willing to deprecate with in the node overrides and go directly to withKlass so that we don't have to construct any intermediate garbage when that's configured.

I think this covers most of the breaking-ish changes I would want to make to the lowest level stuff, in exchange for better DX and no loss of performance (probably better, even if just code size improves).

Why it works

The reason why this works when getType(), exportJSON(), etc. don't work is that here we are returning a Record that can "technically" return configuration for any types.

export type BaseStaticNodeConfig = {
  readonly [K in string]?: StaticNodeConfigValue<LexicalNode, string>;
};

When specialized for a specific class you end up with intersection of any type configuration with one specific optional configuration:

export type StaticNodeConfig<
  T extends LexicalNode,
  Type extends string,
> = BaseStaticNodeConfig & {
  readonly [K in Type]?: StaticNodeConfigValue<T, Type>;
};

For example, something like this:

type CustomText$Config = BaseStaticNodeConfig & {
  readonly 'custom-text'?: {/* ... config */}
};

When you subclass custom-text, its $config type will look something like this:

type MoreCustomText$Config = BaseStaticNodeConfig & {
  readonly 'custom-text'?: {/* ... config */};
  readonly 'more-custom-text'?: {/* ... config */};
};

The types here don't violate any subtype constraint because it's all optional, so the subclass is allowed to return {'more-custom-text: {}} and it still satisfies the superclass type.

Future direction

I think this is where we fix the rest of the #6998 things here with some combination of middleware-style functions or automatic super calls (like $afterCloneFrom) instead of methods so that we can control things better without variance violations.

Copy link

vercel bot commented Feb 27, 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 Mar 4, 2025 6:13am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 4, 2025 6:13am

@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 Feb 27, 2025
Copy link
Member

@zurfyx zurfyx left a comment

Choose a reason for hiding this comment

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

I quite like this proposal this API ticks the inheritance and type-safe customizability boxes, thank you Bob as usual!

I'm not a fan of the repeated extends but The major downside is the fact that this emphasizes JSON structures as opposed to inheritance. This is a shift from our typical paradigm that relies on class based structure. While arguably this $config function is inheritable, the definition of the all properties within this $config happens declarative as part of the initialization, what would have typically been additional properties on the Node.

To put the code in perspective, this is the API proposed in this PR:

return this.config('listitem', {
      $transform: (node: ListItemNode): void => { ... },
      stateConfigs: [
         {flat: true, stateConfig: numberState},
         boolState],
      ],
});

For the sake of $create I don't think this API is needed but I can see how this help, and I would definitely be keen to move toward this direction to double down on the idea behind $applyNodeReplacement that is currently very specific to that job in particular.

Comment on lines +107 to +111
/**
* An alternative to the internal static transform() method
* that provides better DX
*/
readonly $transform?: (node: T) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is a good opportunity to do this: #5572
I don't see why transforms could be registered in the node definition but not any other type of listener or command.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having transform here is mostly because some nodes won’t work correctly at all without their transform, and that there’s already a static method for it. I would prefer to revisit that after we add extensions, since they also cover that functionality.

nodes should really be minimal and only include functionality relevant to the lexical document, IMO they shouldn’t even have DOM code in them, so would be better to not add more to them today. SSR, headless, React Native, Reacf-only rendering, etc. use cases would be easier if nodes didn’t have extra stuff.

Comment on lines +422 to +423
const parentKlass =
config.extends || Object.getPrototypeOf(this.constructor);

This comment was marked as resolved.

@GermanJablo
Copy link
Contributor

So, if I understand correctly, "flat" would be to be able to use this API on existing properties in a backwards compatible way, right?

@etrepum
Copy link
Collaborator Author

etrepum commented Mar 7, 2025

Yes “flat” moves a property out of the $ key to the root of the node’s JSON, allowing for backwards/forwards compatibility of the serialization

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.

Feature: Pre-registration/optional flattening of required NodeState configurations
4 participants