WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Avoid overly-cautious type guards for nodes #5980

@nabbydude

Description

@nabbydude

Problem
There are many parts of the codebase where a function receives a Node and needs to determine whether its a Text, Element, or Editor, which roughly works as follows:

  • Is it a Text?
    • Does it have a text property?
      • Then yes, its a Text. Done 😄
  • Is it an Element?
    • Does it have a children property?
      • Then yes, its a --oh wait it could still be an Editor, gotta make sure its not that first.
  • Is it an Editor?

The current hack to prevent a bloated isEditor subcall for every single isElement call is just to check for the apply function and assume anything with apply is an editor (and therefore not an Element).

// PERF: No need to use the full Editor.isEditor here
const isEditor = typeof value.apply === 'function'
if (isEditor) return false

Solution
I have a few related suggestions:

  1. Codify this check in the schema, that an Element is an object with a children property and without an apply property. (Elements are always JSON, so apply being a function only for editors is already part of the spec, whoops!)
  2. Implement a discriminator function(s) that can assume its input is a Node and only needs to choose between them. This saves on checking types of children, and editor can be checked by just checking the apply property.
    I'm thinking something like:
    function nodeIsText(node: Node): node is Text
    as opposed to:
    function isText(node: any): node is Text
  3. Avoid using isEditor whenever possible, by either comparing the node to a known editor object or checking the length of its path.
  4. (bonus) properly check for all properties of Editor in isEditor

Alternatives
The specific property isn't crucial for my proposal, we could just as easily check for the (non)existence of onChange or operations or shouldMergeNodesRemovePrevNode. It also can be more specific, like specifically checking if its a function. (I really wrote that last part forgetting that it's already the case since Elements are JSON only...)

There are other ways this could be done, like turning Editor into a class or creating some new property/symbol for this check, but to me this way seemed the most in line with the current slate ethos.

Context
Happy to implement this myself, just wanted to hear other thoughts before running off on my own.

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions