-
-
Notifications
You must be signed in to change notification settings - Fork 623
refactor: less use of PartialBlocks and type cleanup #2111
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: refactor/zod-props-v2
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
S extends StyleSchema, | ||
>( | ||
tr: Transaction, | ||
// TBD: allow PartialBlock here? |
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 think we could move it a layer up to the BlockNoteEditor (and only allow partials there, not in the commands. However, for updateBlock
I don't think that's feasible, so maybe better to keep it here
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.
These don't get run anywhere but from the editor instance, so imo it doesn't make too much a difference anyway
* @returns an {@link OccupancyGrid} | ||
*/ | ||
export function getTableCellOccupancyGrid( | ||
block: BlockFromConfigNoChildren<DefaultBlockSchema["table"], 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.
Stopped exporting BlockFromConfigNoChildren
so we have more consistency in the codebase
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.
While we are cleaning up types, I believe that:
BlockFromConfig<DefaultBlockSchema["table"], any, any>
Should only be:
BlockFromConfig<DefaultBlockSchema["table"]>
Most of the time we don't care about the other parameters, and have to remove their defaults with any. This goes for most instances of B, I, S
(props as any)[name] = spec._zod.def.defaultValue; | ||
} | ||
} | ||
// const props = block.props || {}; |
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.
TODO: this code can now be deleted
StyleSpecs, | ||
import { | ||
partialBlockToFullBlock, | ||
type BlockIdentifier, |
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, some of the changes in type
imports were to fix a circular dependency issue that was breaking the build during development
*/ | ||
public blocksToHTMLLossy( | ||
blocks: PartialBlock<BSchema, ISchema, SSchema>[] = this.document, | ||
blocks: Block<BSchema, ISchema, SSchema>[] = this.document, |
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.
breaking API change, should be documented in PR / release notes
} | ||
|
||
// Hide handles if the table block has been removed. | ||
this.state.block = this.editor.getBlock(this.state.block.id)!; |
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.
TODO: In the old version we were potentially setting this.state.block
to undefined. Now, it will keep the old (unavailable block). can this break anything?
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.
It isn't clear to me why this was modified, but it certainly is a change in behavior that would need to be validated
* | ||
*/ | ||
|
||
export type PartialTableCell< |
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.
moved to separate table file
children: PartialBlockNoDefaults<BSchema, I, S>[]; | ||
}>; | ||
|
||
export type SpecificPartialBlock< |
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.
unused, removed
} | ||
: { | ||
type: "tableCell", | ||
// FIXME: content can actually be Partial, we should probably handle this as well |
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.
This comment is to document a finding Matthew earlier found while writing the partialToFull function
"editor.defaultFormatter": "esbenp.prettier-vscode", | ||
"editor.formatOnSave": true, | ||
"editor.codeActionsOnSave": { | ||
"source.addMissingImports": "explicit" |
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, this makes sure missing imports are added automatically on save, I kept finding myself to do this manually (or AI-powered)
S extends StyleSchema, | ||
>( | ||
tr: Transaction, | ||
// TBD: allow PartialBlock here? |
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.
These don't get run anywhere but from the editor instance, so imo it doesn't make too much a difference anyway
// for this, we do a nodeToBlock on the existing block to get the children. | ||
// it would be cleaner to use a ReplaceAroundStep, but this is a bit simpler and it's quite an edge case | ||
const existingBlock = nodeToBlock(blockInfo.bnBlock.node, pmSchema); | ||
const newFullBlock = partialBlockToFullBlock(schema, block); |
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.
const newFullBlock = partialBlockToFullBlock(schema, block); | |
const newBlock = partialBlockToFullBlock(schema, block); |
There are partial blocks & blocks. Full block gives a third name which is not helpful
// TODO: what happened here? | ||
return isCellEmpty(cell); |
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.
Is this wrong? It is recursing, what you've written is an infinite loop no?
* @returns an {@link OccupancyGrid} | ||
*/ | ||
export function getTableCellOccupancyGrid( | ||
block: BlockFromConfigNoChildren<DefaultBlockSchema["table"], 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.
While we are cleaning up types, I believe that:
BlockFromConfig<DefaultBlockSchema["table"], any, any>
Should only be:
BlockFromConfig<DefaultBlockSchema["table"]>
Most of the time we don't care about the other parameters, and have to remove their defaults with any. This goes for most instances of B, I, S
const domFragment = serializeInlineContentExternalHTML( | ||
editor, | ||
inlineContent as any, | ||
inlineContent as unknown as never, |
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.
~~ as never
? Why so? ~~
nvm, I see the todo
let id = block.id; | ||
|
||
if (id === undefined) { | ||
id = UniqueID.options.generateID(); | ||
} | ||
|
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'm actually ok with this function generating ids if the block doesn't have one. I think this makes the caller's life more difficult to be forced to present an ID
blockHasType( | ||
block, | ||
this.editor, | ||
"table", | ||
defaultBlockSpecs.table.config.propSchema, | ||
) |
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.
Why are we changing this?
Am I right to assume that this would mean that it can now only match the block if it is both a table & conforming to the propSchema? Why does the propSchema matter here? What if someone extends the table to have additional props, wouldn't this break?
} | ||
|
||
// Hide handles if the table block has been removed. | ||
this.state.block = this.editor.getBlock(this.state.block.id)!; |
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.
It isn't clear to me why this was modified, but it certainly is a change in behavior that would need to be validated
// TODO: I think this should be a CustomBlockNoteSchema, | ||
// but that breaks docxExporter etc |
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.
Yep, it should be
Summary
Clean types and Partial / full block concepts
Rationale
#2089 (review)
Changes
PartialBlock
internally, where it's not necessaryImpact
Testing
Should mostly be covered by existing unit tests
Screenshots/Video
Checklist
Additional Notes
TODO:
Follow up: