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

consider removing all object properties #2757

Closed
ianstormtaylor opened this issue May 10, 2019 · 1 comment
Closed

consider removing all object properties #2757

ianstormtaylor opened this issue May 10, 2019 · 1 comment

Comments

@ianstormtaylor
Copy link
Owner

Do you want to request a feature or report a bug?

Idea / improvement / debt.

What's the current behavior?

Right now we have .object properties on almost all Slate-specific objects. (With the exception of paths I think, since they are arrays.) This kind of just developed because we'd serialize the "instance" to JSON and preserve the .object property. And it was deemed necessary for nodes to distinguish between blocks, inlines and documents (and somewhat texts), so we just did it for everything for consistency.

What's the expected behavior?

But with the coming move to plain JSON objects/arrays for representing data, requiring .object properties because more tedious. And if we switch to interface-based helpful functions as mentioned in #2345 (comment) then there will be even less need for the .object properties.

The only case where it would be currently still required, would be to distinguish block, inline and document nodes.

Which got me thinking about how the DOM avoids this natively by using CSS. And we've avoided properties like this already by using editor-level queries like isVoid or isAtomic for what are considered "runtime" decisions. We could actually do the same thing with a new isInline property on nodes.

That eliminates the block vs. inline need. But the remaining issue becomes how to handle the document node. I'm not entirely sure without investigating more, but we could potentially treat it as just another block node without special privileges. This might work, although I think there are places where it's helpful to know if we're at the root-level document.

Another approach could be to instead make the value itself implement the Node interface, by changing the "document" to be an array on the value.nodes property. This would nicely eliminate the awkward duplication of value.data and document.data as well, which tend to be used interchangeably as far as the data model goes. And it would eliminate the need to constantly reach down into value.document to do things like:

for (const [block, path] of Node.blocks(value.document)) {
  // ...
}

(Thinking even further, that second solution makes you wonder if the value should also implement the Range interface as a way to eliminate the need to drop into value.selection all over the place as well... food for thought. Even less sure about this one though.)

@ianstormtaylor
Copy link
Owner Author

Pretty sure this is the way to go, so I've opened issues to make it happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant