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

simplify schema error context properties #2983

Closed
ianstormtaylor opened this issue Aug 27, 2019 · 3 comments
Closed

simplify schema error context properties #2983

ianstormtaylor opened this issue Aug 27, 2019 · 3 comments

Comments

@ianstormtaylor
Copy link
Owner

ianstormtaylor commented Aug 27, 2019

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

Improvement.

What's the current behavior?

Right now schema errors contain a lot of properties related to the nodes that they are referring to. For example, child, index, parent, node, previous, next.

This is alright, but it's kind of hard to keep track of and seems unnecessary. Once we add path-based support to schema rules, a lot of these things could be determined in userland more easily. For example, given a child_type_invalid error, right now you get:

{
  child: Node,
  index: Number,
  node: Node,
  rule: Object,
}

But instead, if you only received:

{
  path: Array,
  rule: Object,
}

Where path was a reference to the child that failed validation. The child, parent and index could be inferred from those two pieces of data. The same goes for accessing the previous siblings, first children, etc. from other schema errors.

So I think changing schema errors to simply return a path to the node that failed validation makes things a lot cleaner. (And in some cases extra information, like the limit for "max/min" validations.)

@cameracker
Copy link
Collaborator

I think one upside to having the descriptive property names its more obvious what means what when a user is trying to write a normalizer for a rule violation. In the child_type_invalid example, I know exactly what node is the child, and a little logical jump can help me figure out what the parent is by elimination. With a path, it's a little less clear because we don't have the name to assist.

I think success for this change would require a convention that all path properties refer to the node referenced in the name of the rule, for example

  • child_type_invalid: points to child node, like suggested
  • parent_type_invalid: points to the parent node
  • next_sibling_type_invalid: points to the sibling
  • previous_sibling_type_invalid: points to the sibling

The sibling rules feel a little weird to me...

@ianstormtaylor
Copy link
Owner Author

That’s how I was thinking about it too. The only exception (unfortunately) is that for parent_* errors the path points to the child that defined the rule.

But I’ve got it working on that key-less branch and it feels nicer. Especially once we can actually get rid of all the other properties in a future breaking change it’ll clean up the schema a lot.

And once keys are removed, like 90% of the work of these normalizing functions is going to revolve around the error’s path.

@ianstormtaylor ianstormtaylor mentioned this issue Nov 6, 2019
@ianstormtaylor
Copy link
Owner Author

I believe that this may be fixed by #3093, which has changed a lot of the logic in Slate and slate-react especially. I'm going to close this out, but as always, feel free to open a new issue if it persists for you. Thanks for understanding.

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

Successfully merging a pull request may close this issue.

2 participants