-
-
Notifications
You must be signed in to change notification settings - Fork 623
refactor: zod for props #2096
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
base: main
Are you sure you want to change the base?
refactor: zod for props #2096
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
// TBD: this might not be fault proof, but it's also ugly to store prop=""..."" for strings | ||
try { | ||
const jsonValue = JSON.parse(value); | ||
// it was a number / boolean / json object stored as attribute | ||
return z.parse(spec, jsonValue); | ||
} catch (e) { | ||
// it might have been a string directly stored as attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend https://github.com/unjs/destr & maybe this is helpful? https://zod.dev/codecs?id=jsonschema
const def = | ||
spec instanceof z.$ZodDefault ? spec._zod.def.defaultValue : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen this in a number of places, would prefer to make a util function for this, since it isn't obvious & dependent on zod's API not our own. I don't love reach into zod for this, but don't see another way to do this unless we provide defaults separately
"backgroundColor": "default", | ||
"caption": "", | ||
"name": "", | ||
"previewWidth": undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, when getting a block you now also get optional props as undefined. I think this is ok?
cc @nperez0111
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me, probably better even.
} | ||
|
||
try { | ||
// TODO: props.level type should propagate correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nperez0111 I didn't get this to work with the way blocks are defined. Can you have a look at this or can we pair up?
const BC_NODE = editor.pmSchema.nodes["blockContainer"]; | ||
|
||
// set default props in case we were passed a partial block | ||
// TODO: should be a nicer way for this / or move to caller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi, I don't think these functions should take PartialBlocks. I want to review the PartialBlock concept later. My thought at this moment is that they should only be on the highest level, to make calling things like updateBlock
/ insertBlocks
more convenient
import { Block } from "./defaultBlocks.js"; | ||
import { Selection } from "prosemirror-state"; | ||
|
||
// TODO: matthew review + tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewlipski , you recently refactored this file I think. afaik there are no unit tests covering the type guards. Could you add these? I'm not sure if my implementation matches the scenarios you had in mind
You can also add them to the main
branch implementation and then I can migrate them to validate they still work after this zod refactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests in #2103
*/ | ||
comments?: { | ||
schema?: BlockNoteSchema<any, any, any>; | ||
schema?: CustomBlockNoteSchema<any, any, any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nperez0111 please double check my changes in this file (not sure I correctly get the difference between CustomBlockNoteSchema and BlockNoteSchema)
>( | ||
options?: Options, | ||
): Options extends { | ||
schema: CustomBlockNoteSchema<infer BSchema, infer ISchema, infer SSchema>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nperez0111 another schema change
@YousefED so with this PR are we introducing the ability to have objects as props alongside strings/numbers/booleans? Because I remember this was something we considered when first making the custom blocks API, but skipped it mainly because objects need to be set on HTML attributes with |
Summary
This PR drops our custom schema for Block and Inline Content Props and uses Zod instead.
Rationale
Changes
Impact
Consumers using custom blocks would need to migrate
Testing
Mostly by existing unit tests. TODO:
BlockNoteSchema
vsCustomBlockNoteSchema
. Perhaps @nperez0111 can help here?Screenshots/Video
N/A
Checklist
Additional Notes
Future work: