Added functionality for setNode to take a function as props argument

[adisen]

Description
This PR allows Transforms.setNodes to take either a Partial as it second argument to props (which we have before) which overrides the props on the node or a function that takes the existing props for the relevant node and returns a new set of props to attach to the node.

Issue
Fixes: #3808

Checks

• The new code matches the existing patterns and styles.
• The tests pass with yarn test.
• The linter passes with yarn lint. (Fix errors with yarn fix.)
• The relevant examples still work. (Run examples with yarn start.)

[changeset-bot]

⚠️ No Changeset found

Latest commit: bbc3b30

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[adisen]

@BrentFarese check here. The NodeProps type you asked me to adjust didn't work.

[TheSpyder]

This doesn't look like it fixes #3808 completely, there's a secondary split issue mentioned?

..... oh, it seems the split was fixed in #4168 🤔 so maybe that's ok

[ianstormtaylor]

Hey @adisen there's an issue with the TypeScript linting here. Are you able to fix it? If not, can you make your branch editable so that I can make changes to the PR? Thanks!

[TheSpyder]

It's the change from Partial<Node> to Partial<T extends Node>. I've found that when T is defined as something like ImageElement | LinkElement | VideoElement, TypeScript only checks the first of the T definitions when it's used inside a wider multi-type definition such as Partial<T> | ((existingProps: NodeProps) => Partial<T>).

Hence the error that 'link' is not assignable to 'image'.

Switching setNodes to Partial<Node> | ((existingProps: NodeProps) => Partial<Node>) should fix it.

[BrentFarese]

It's the change from Partial<Node> to Partial<T extends Node>. I've found that when T is defined as something like ImageElement | LinkElement | VideoElement, TypeScript only checks the first of the T definitions when it's used inside a wider multi-type definition such as Partial<T> | ((existingProps: NodeProps) => Partial<T>).

Hence the error that 'link' is not assignable to 'image'.

Switching setNodes to Partial<Node> | ((existingProps: NodeProps) => Partial<Node>) should fix it.

If there is any way to preserve the generic type, that might be preferred. What we want is for the callback to setNodes to be aware of the specific NodeProps available to it. Makes for a much better TS experience as now I can know what exactly the shape of existingProps is. Maybe there is some way to solve the type issue w/o removing the generic.

[TheSpyder]

Perhaps it could be Partial<Node> | ((existingProps: NodeProps) => Partial<T>) but I think it would just lead to the same issue, only for people who use the function.

Specifying a concrete T in the calling type (change const newProperties: Partial<SlateElement> to const newProperties: Partial<LinkElement> in embeds.tsx line 73) also resolves this but that's a breaking change.

[ianstormtaylor]

I think going without the generic for now is best, because I found that there were some weird gotchas in converting to generics in my other PR. Especially in cases like this where the generic was trying to be used for match as well.

[absolutejam]

Is the scope of this issue limited to still returning the same node type - ie. (T) => T - or would it facilitate changing node type?

I'm just coming back to a side-project that uses Slate and I've written an F# wrapper for (most of) the Slate API, but one of the things that causes me no end of issues is that setNodes changes the shape of the node instead of straight up replacing the node.

I find myself often having to mutate the object and manually juggle types, but realistically - for use in TypeScript or F# - I'd want to completely transform from ParagraphNode to TitleNode, even if that means creating a new object (ensuring there are no unaccounted-for properties), but I don't know if this would play havoc with how Slate manages its internal state.

EDIT: Or maybe I'm just handling this wrong and should be using another pattern (unwrap and wrap?)

[TheSpyder]

Is the scope of this issue limited to still returning the same node type - ie. (T) => T - or would it facilitate changing node type?

setNodes can change anything except the children or text properties. So changing between block and inline types is allowed but it can't do to or from text node.

I'm just coming back to a side-project that uses Slate and I've written an F# wrapper for (most of) the Slate API

Oh interesting! My team have created bindings in ReScript. Is yours open source? Might be fun to compare notes 🤔

one of the things that causes me no end of issues is that setNodes changes the shape of the node instead of straight up replacing the node.

I find myself often having to mutate the object and manually juggle types, but realistically - for use in TypeScript or F# - I'd want to completely transform from ParagraphNode to TitleNode, even if that means creating a new object (ensuring there are no unaccounted-for properties), but I don't know if this would play havoc with how Slate manages its internal state.

setNodes is designed for modifying attributes. Technically it creates a new object to do this, since the Slate model is immutable, but the apparent effect is that it's mutating the node.

EDIT: Or maybe I'm just handling this wrong and should be using another pattern (unwrap and wrap?)

Possibly. Wrap and unwrap will work, it's just a little more effort.

[dylans]

@adisen are you still interested in this PR? If so, it needs a rebase and there's some feedback to address. Thanks!

[dylans]

Closing due to inactivity. Feel free to reopen or re-raise if there's interest.