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

Feat: (proposal) prop validation. #271

Closed
wants to merge 11 commits into from

Conversation

leweyse
Copy link

@leweyse leweyse commented Jul 11, 2023

Hi @matthewlipski. I've been working on a proposal for the propSchema validation, and I hope I can get your feedback. This is still a work in progress, so I'll be happy to discuss any suggestions or changes. This will also fix #264 .

I think the best fit for schema validation and type inference is zod. So, I worked on an implementation for the propSchema definition using zod. This is how it would look like: https://github.com/Leweyse/BlockNote/blob/feat/prop-validation/examples/editor/src/App.tsx#L10. This will make possible to parse the props values and also enable autocomplete when we use helpers as updateBlock or insertBlocks (example here). It will also simplify the type definition from the library, but keeping the app type-safe: https://github.com/Leweyse/BlockNote/blob/feat/prop-validation/packages/core/src/extensions/Blocks/api/blockTypes.ts#L45.

@vercel
Copy link

vercel bot commented Jul 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
blocknote ✅ Ready (Inspect) Visit Preview Jul 11, 2023 8:46am
blocknote-website ✅ Ready (Inspect) Visit Preview Jul 11, 2023 8:46am

@matthewlipski
Copy link
Collaborator

Looks interesting! It seems like using zod internally makes sense to ensure a strict schema, though one thing I'm a bit concerned about is exposing the zod API to consumers. I think ideally, you should be able to declare a prop schema using a vanilla TypeScript object. I'm not familiar with zod myself, but do you think it would be possible to enforce strict typing without having to declare the prop schema using zod functions?

@leweyse
Copy link
Author

leweyse commented Jul 11, 2023

Looks interesting! It seems like using zod internally makes sense to ensure a strict schema, though one thing I'm a bit concerned about is exposing the zod API to consumers. I think ideally, you should be able to declare a prop schema using a vanilla TypeScript object. I'm not familiar with zod myself, but do you think it would be possible to enforce strict typing without having to declare the prop schema using zod functions?

Fair point. I think that the approach of TRPC makes sense for this case: https://trpc.io/docs/server/validators#the-most-basic-validator-a-function. It enables the posibility to use various validators: simple function, zod schema, yup and others. It will depend on the user what they would like to use. Their implementation seems to be pretty straightforward:

What do you think about this approach?

@YousefED
Copy link
Collaborator

Hi @leweyse,

Thanks for your work on this. Impressive work! I have some thoughts / questions on this. Want to make sure we have the picture clear before deciding on a direction :)

Understanding the motivation
Can you clarify what the exact motivation is? It seems to me there are two or three things you'd like to address:

  1. Support for complex prop types
    You'd like to use props for custom blocks that are not limited to the string type (i.e.: objects, arrays, etc). I think this makes sense. This is your primary motivation, right?

  2. Type safety of props (current release)
    Do you have issues with the type-safety of (string) props in the current release? Afaik regular props and calls to updateBlock etc. should already be type safe. Or are you running into issues / limitations for this?

  3. Runtime validation using zod
    You'd like to use zod for runtime validation of props. afaik we currently don't do this, but of course things are somewhat guarded at compile-time by the type system. Can you explain your motivation for runtime validation further?

Unknowns re. complex prop types
BlockNote uses Prosemirror as underlying framework. Props are eventually translated to prosemirror node attributes (attrs). I think we need to research / answer the following questions (in the order below) before moving ahead with more complex attrs, to make sure they're fully supported:

  1. Do complex types (arrays, objects, nesting) work well with Prosemirror? It's not very common to see this, at least I can't think of any project using prosemirror that uses non-primitive attributes
  2. Do complex types work well with y-prosemirror? BlockNote uses y-prosemirror for collaboration. Because complex attrs are not common in Prosemirror, it wouldn't surprise me if y-prosemirror hasn't been designed for this. We'd need to confirm this before moving ahead with this.
  3. Serialization of props. As @matthewlipski mentioned in Fix: propSchema data-types and TS autocomplete. #264, props are serialized to the DOM, and this serialization is also used when copy/pasting. I think we could probably use JSON for serialization, so this doesn't have to be a dealbreaker, but it's one other thing that would need to be implemented and tested. (note that serialization of custom blocks is a WIP anyway at feat: Custom block serialization #257)

Hope this makes sense. Let me know if you have any questions on the above!

@leweyse
Copy link
Author

leweyse commented Jul 18, 2023

Hi @YousefED,

Thanks for your input.

Understanding the motivation

  1. That's right. I've been working with Custom Blocks, and updating their props in the editor (e.g. styles, content, etc.) is required for my use case. Some of the components that I have to use have objects as props, so I'd like to ensure their use.
  2. No, everything is in order with string type props. With the work on this branch, I try to extend the type-safety to the new prop types. One additional thing that I addressed here: updateBlock currently uses a Partial type from propSchema, but I think it would be better to let the user decide which properties are optional and which are not.
  3. Actually my main motivation to use zod is the great type inference that it provides, but I also think that validating objects and arrays will become way easier. I have in mind some use cases for runtime validation. For example, the custom block Image from the docs expects a string for the src prop, but ideally we should make sure that the user input is actually a valid URL. Using zod, we can rely on z.string().url() for this situation. I guess we could still discuss whether BlockNote or the user should handle this.

Unknowns re. complex prop types

  1. and 2. It seems that non-primitive attributes are not expected by Prosemirror. What about if BlockNote only renders nested elements if they are primitive data types? The name of those attributes could be determined by the structure of the object or array.
E.g. objects:

name: {
  first: {
    second: 'test1',
    third: 'test2'
  }
}

<div data-name.first.second="test1" data-name.first.third="test2" />

E.g. arrays:

arr: ['first', 'second', 'third']

<div data-arr--0="first" data-arr--1="second" data-arr--2="third" />

It's just an idea, but those attribute names are valid (source).

  1. I didn't think about it. Maybe we can split the functionality of propSchema. A new props property could be used to provide the default values, and the propSchema to provide a parse function. I'm sure this is not completely required, so I'm just giving ideas.

Let me know what you think, I'll be glad to continue working on these changes.

@leweyse leweyse marked this pull request as draft July 18, 2023 20:26
@YousefED
Copy link
Collaborator

Clear! Thanks.

Based on your input I suggest the following:

Actually my main motivation to use zod is the great type inference that it provides, but I also think that validating objects and arrays will become way easier.

If validation is not the main goal, and the types can be enforced by raw Typescript, then I think we should focus on doing that without Zod. We can solve the runtime validation issue separately - imo better to address one problem at a time.

and 2. It seems that non-primitive attributes are not expected by Prosemirror.

I'm still not sure about this. The example you give is about serializing to / from DOM, but perhaps PM and y-pm do work well with more complex attrs (except for the serialization). Have you tested this (separate from the serialization)?

Re your proposal; if we want to serialize more complex props to / from DOM, I think it would be easier (from a maintainability perspective) to embed them as serialize / deserialize as raw JSON then inventing a new structure tbh

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

Successfully merging this pull request may close these issues.

None yet

3 participants