-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add Custom TypeScript Types to Slate #3725
Comments
@ianstormtaylor @CameronAckermanSEL @ccorcos @BrentFarese (and anybody else) would be interested in getting some feedback. Additional thoughts:
|
I think this is a good proposal to make declaration merging a bit better. Ultimately, I think Slate should support both generics and declaration merging but declaration merging is the "smaller lift" in terms of refactors to Slate core. So, I would be in favor of implementing something like the above somewhat soon so we all get type safety on I'll let others chime in but do you want to open up a branch for the work @thesunny? Happy to contribute and collaborate on a review... We are releasing our Slate editor somewhat soon and it'd be great to have full type safety before then. We will continue to work on the generics solution too but I think this is a good stop-gap that may provide the same type safety as generics. |
I was pretty excited about this proposal catching 90% of the use cases but I'm leaning towards the value of having both. There are use cases where generics are the only solution like a method that returns the children of a Node. If the Node passed in is known, the generic can return the appropriate type. For example, One benefit of having both is that (a) they can work together and (b) you can choose the trade offs that work for you. They don't interfere with each other and generics can benefit from having custom I'd be happy to open a branch or @BrentFarese I'd be just as happy if you wanted to go ahead and do this yourself and cut and paste the code. I am a little hesitant as this feels like a big decision to not involve @ianstormtaylor in before starting. I wonder if we should wait for a go ahead. |
I think we should go ahead @thesunny. @CameronAckermanSEL has talked to @ianstormtaylor and I think Ian is supportive of the generics option, no? As long as this isn't limiting of generics, which I don't think it is at all (it's complimentary), I think it would be good to have. We can also have it sooner than generics in my opinion. Cam or Ian can chime in but I think we go for it. Open a branch and add me as a contributor? @timbuckley from our team can contribute too. Thanks! |
Here's a link to the branch https://github.com/ianstormtaylor/slate/tree/declaration-merging I haven't submitted any code. Please feel free to start without me and use the code I posted. If you decide to go ahead, I recommend renaming |
Just FYI some fellows from MLH are working on implementing this. We can share a branch soon. I think the issue of not being able to have 2 different sets of types for multiple editors is acceptable because a user that has multiple editors can just include custom types for all editors the user might have, which is better than the current type system. It's not perfect, but definitely better than the current state. |
Here is the branch where the work is occurring. https://github.com/arity-contracts/slate/tree/custom-extensions. If anyone wants to be added to help out, let us know but I think we have enough resources to complete this. |
Actually due to some issues the branch is now https://github.com/arity-contracts/slate/tree/custom-types Sorry! |
This PR adds better TypeScript types into Slate and is based on the proposal here: #3725 * Extend Slate's types like Element and Text * Supports type discrimination (ie. if an element has type === "table" then we get a reduced set of properties) * added custom types * files * more extensions * files * changed fixtures * changes eslint file * changed element.children to descendant * updated types * more type changes * changed a lot of typing, still getting building errors * extended text type in slate-react * removed type assertions * Clean up of custom types and a couple uneeded comments. * Rename headingElement-true.tsx.tsx to headingElement-true.tsx * moved basetext and baselement * Update packages/slate/src/interfaces/text.ts Co-authored-by: Brent Farese <25846953+BrentFarese@users.noreply.github.com> * Fix some type issues with core functions. * Clean up text and element files. * Convert other types to extended types. * Change the type of editor.marks to the appropriate type. * Add version 100.0.0 to package.json * Revert "Add version 100.0.0 to package.json" This reverts commit 329e44e. * added custom types * files * more extensions * files * changed fixtures * changes eslint file * changed element.children to descendant * updated types * more type changes * changed a lot of typing, still getting building errors * extended text type in slate-react * removed type assertions * Clean up of custom types and a couple uneeded comments. * Rename headingElement-true.tsx.tsx to headingElement-true.tsx * moved basetext and baselement * Update packages/slate/src/interfaces/text.ts Co-authored-by: Brent Farese <25846953+BrentFarese@users.noreply.github.com> * Fix some type issues with core functions. * Clean up text and element files. * Convert other types to extended types. * Change the type of editor.marks to the appropriate type. * Run linter. * Remove key:string uknown from the base types. * Clean up types after removing key:string unknown. * Lint and prettier fixes. * Implement custom-types Co-authored-by: mdmjg <mdj308@nyu.edu> * added custom types to examples * reset yarn lock * added ts to fixtures * examples custom types * Working fix * ts-thesunny-try * Extract interface types. * Fix minor return type in create-editor. * Fix the typing issue with Location having compile time CustomTypes * Extract types for Transforms. * Update README. * Fix dependency on slate-history in slate-react Co-authored-by: mdmjg <mdj308@nyu.edu> Co-authored-by: Brent Farese <brentfarese@gmail.com> Co-authored-by: Brent Farese <25846953+BrentFarese@users.noreply.github.com> Co-authored-by: Tim Buckley <timothypbuckley@gmail.com>
This is now available in the To install, use |
Cool feature, any plan on making an official release @thesunny ? |
IMPORTANT! Due to an issue when switching from the I've reopened this issue. For more details, please refer to #4003 |
This is now available in the @next release. Use |
Hello everyone, we could use some help by having people try out the new TypeScript types in their own editors. They are available by using an Once you’ve integrated it, please let us know by posting below. We want to get this into the regular npm packages as quickly as possible. This has the benefit of getting better types into Slate, but also has the added benefit of being able to start accepting more PRs. Ian has added more maintainers and given out more publishing rights so this should allow us to accept more PRs faster into Slate. |
Thanks for the progress on this @thesunny and for soliciting community input.
I'm testing these types in Luma. You can see an interactive version of our editor here: https://lu.ma/play-editor I am getting a lot of TypeScript errors while testing this new release. I have updated my types file to this gist: https://gist.github.com/vpontis/fc783570a04d2d9e14c748002f7d3ae2 Here is one of my components: export const SlateLeaf = ({ attributes, children, leaf }: RenderLeafProps) => {
if (leaf.bold) {
children = <strong>{children}</strong>;
}
if (leaf.code) {
children = <code>{children}</code>;
}
if (leaf.italic) {
children = <em>{children}</em>;
}
if (leaf.underline) {
children = <u>{children}</u>;
}
return <span {...attributes}>{children}</span>;
}; Then, when compiling the TS:
|
Hi @thesunny and thanks for your work. I tested it on my code base. Two things:
I get an error much like @vpontis saying:
where one can see that there is no need to say that heading or list-item have a children property. I understand that your code "injects" that prop automatically. That works fine but my codebase needs a TS interface to the whole definition for each element. Using the main slate release I have something like that:
I managed to achieve the same thing with your TS definition:
then I override CustomTypes:
Is this the "correct" way ? |
@thesunny I'm surprised by the way that we are extending types. I haven't seen any other package require you to do Would it make sense to do an API like this: const editor = createEditor<CustomElement, CustomLeaf>();
type CustomRenderLeafProps = RenderLeafProps<Override> |
For the record and to give credits where it's due @BrentFarese, @timbuckley @mdmjg did the hard work. I did the initial design and helped a bit at the end. I @mentioned them to get them into this conversation. I think they will do a better job at answering any questions. I haven't integrated the new types in my app at the moment. |
Yes this is the "correct" way to extend the types. I notice in your custom types definition, it doesn't seem like you have defined paragraph. Have you defined paragraph in your types @htulipe? Also I would encourage this discussion on the #typescript channel in Slate Slack vs. here on GitHub. Thanks! |
Hi @BrentFarese, it's only a excerpt of my code, I do have paragraph defined. I'll ask about my TS error on the Slack channel 👍 |
This is just a little feedback from a new user of this PR... As a relatively new users I was a little confused when switching to @next, since all of the examples in Anyway, the above didn't take too long, and now that I understand how this PR works, I really like it (THANKS!). But definitely a blocker on the todo list for releasing the next version of slate should be to fix the examples, and update the docs, or find a way to make this explicit typing optional and turn that on by default. I'm not volunteering, and again I greatly appreciate all the work you have already done on this! |
i am testing the new version for the upcoming major update of react-page (see https://github.com/react-page/react-page/tree/beta) i have to yet find out if the Luckely for our library, it will be hidden for the consumer of our library, thats what counts. keep you updated with feedback Edit 1: it does not seem to work for Work in progress: react-page/react-page#890 i had to fallback to any casts, i could not figure out how to do it problerly, maybe someone can help? |
@thesunny The docs and src code says that the transformer types is
However, the node_modules/slate shows all transform methods taks export declare const Transforms: {
delete: (editor: import("..").BaseEditor, options?: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
distance?: number | undefined;
unit?: "character" | "word" | "line" | "block" | undefined;
reverse?: boolean | undefined;
hanging?: boolean | undefined;
voids?: boolean | undefined;
} | undefined) => void;
insertFragment: (editor: import("..").BaseEditor, fragment: import("..").BaseNode[], options?: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
hanging?: boolean | undefined;
voids?: boolean | undefined;
} | undefined) => void;
insertText: (editor: import("..").BaseEditor, text: string, options?: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
voids?: boolean | undefined;
} | undefined) => void;
collapse: (editor: import("..").BaseEditor, options?: {
edge?: "anchor" | "focus" | "start" | "end" | undefined;
} | undefined) => void;
deselect: (editor: import("..").BaseEditor) => void;
move: (editor: import("..").BaseEditor, options?: {
distance?: number | undefined;
unit?: "character" | "word" | "line" | "offset" | undefined;
reverse?: boolean | undefined;
edge?: "anchor" | "focus" | "start" | "end" | undefined;
} | undefined) => void;
select: (editor: import("..").BaseEditor, target: import("..").Location) => void;
setPoint: (editor: import("..").BaseEditor, props: Partial<import("..").BasePoint>, options?: {
edge?: "anchor" | "focus" | "start" | "end" | undefined;
} | undefined) => void;
setSelection: (editor: import("..").BaseEditor, props: Partial<import("..").BaseRange>) => void;
insertNodes: (editor: import("..").BaseEditor, nodes: import("..").BaseEditor | import("..").BaseElement | import("..").BaseText | import("..").BaseNode[], options?: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
match?: ((node: import("..").BaseNode) => boolean) | undefined;
mode?: "highest" | "lowest" | undefined;
hanging?: boolean | undefined;
select?: boolean | undefined;
voids?: boolean | undefined;
} | undefined) => void;
liftNodes: (editor: import("..").BaseEditor, options?: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
match?: ((node: import("..").BaseNode) => boolean) | undefined;
mode?: "highest" | "lowest" | "all" | undefined;
voids?: boolean | undefined;
} | undefined) => void;
mergeNodes: (editor: import("..").BaseEditor, options?: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
match?: ((node: import("..").BaseNode) => boolean) | undefined;
mode?: "highest" | "lowest" | undefined;
hanging?: boolean | undefined;
voids?: boolean | undefined;
} | undefined) => void;
moveNodes: (editor: import("..").BaseEditor, options: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
match?: ((node: import("..").BaseNode) => boolean) | undefined;
mode?: "highest" | "lowest" | "all" | undefined;
to: import("..").Path;
voids?: boolean | undefined;
}) => void;
removeNodes: (editor: import("..").BaseEditor, options?: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
match?: ((node: import("..").BaseNode) => boolean) | undefined;
mode?: "highest" | "lowest" | undefined;
hanging?: boolean | undefined;
voids?: boolean | undefined;
} | undefined) => void;
setNodes: (editor: import("..").BaseEditor, props: Partial<import("..").BaseEditor> | Partial<import("..").BaseElement> | Partial<import("..").BaseText>, options?: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
match?: ((node: import("..").BaseNode) => boolean) | undefined;
mode?: "highest" | "lowest" | "all" | undefined;
hanging?: boolean | undefined;
split?: boolean | undefined;
voids?: boolean | undefined;
} | undefined) => void;
splitNodes: (editor: import("..").BaseEditor, options?: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
match?: ((node: import("..").BaseNode) => boolean) | undefined;
mode?: "highest" | "lowest" | undefined;
always?: boolean | undefined;
height?: number | undefined;
voids?: boolean | undefined;
} | undefined) => void;
unsetNodes: (editor: import("..").BaseEditor, props: string | string[], options?: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
match?: ((node: import("..").BaseNode) => boolean) | undefined;
mode?: "highest" | "lowest" | "all" | undefined;
split?: boolean | undefined;
voids?: boolean | undefined;
} | undefined) => void;
unwrapNodes: (editor: import("..").BaseEditor, options?: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
match?: ((node: import("..").BaseNode) => boolean) | undefined;
mode?: "highest" | "lowest" | "all" | undefined;
split?: boolean | undefined;
voids?: boolean | undefined;
} | undefined) => void;
wrapNodes: (editor: import("..").BaseEditor, element: import("..").BaseElement, options?: {
at?: import("..").Path | import("..").BasePoint | import("..").BaseRange | undefined;
match?: ((node: import("..").BaseNode) => boolean) | undefined;
mode?: "highest" | "lowest" | "all" | undefined;
split?: boolean | undefined;
voids?: boolean | undefined;
} | undefined) => void;
transform: (editor: import("..").BaseEditor, op: import("..").Operation) => void;
};
//# sourceMappingURL=index.d.ts.map If I import |
i think i am running in the same problem. on a side note: how can i do this declaration once in my project? i had to repeat the declaration in any file. |
This PR adds better TypeScript types into Slate and is based on the proposal here: ianstormtaylor#3725 * Extend Slate's types like Element and Text * Supports type discrimination (ie. if an element has type === "table" then we get a reduced set of properties) * added custom types * files * more extensions * files * changed fixtures * changes eslint file * changed element.children to descendant * updated types * more type changes * changed a lot of typing, still getting building errors * extended text type in slate-react * removed type assertions * Clean up of custom types and a couple uneeded comments. * Rename headingElement-true.tsx.tsx to headingElement-true.tsx * moved basetext and baselement * Update packages/slate/src/interfaces/text.ts Co-authored-by: Brent Farese <25846953+BrentFarese@users.noreply.github.com> * Fix some type issues with core functions. * Clean up text and element files. * Convert other types to extended types. * Change the type of editor.marks to the appropriate type. * Add version 100.0.0 to package.json * Revert "Add version 100.0.0 to package.json" This reverts commit 329e44e. * added custom types * files * more extensions * files * changed fixtures * changes eslint file * changed element.children to descendant * updated types * more type changes * changed a lot of typing, still getting building errors * extended text type in slate-react * removed type assertions * Clean up of custom types and a couple uneeded comments. * Rename headingElement-true.tsx.tsx to headingElement-true.tsx * moved basetext and baselement * Update packages/slate/src/interfaces/text.ts Co-authored-by: Brent Farese <25846953+BrentFarese@users.noreply.github.com> * Fix some type issues with core functions. * Clean up text and element files. * Convert other types to extended types. * Change the type of editor.marks to the appropriate type. * Run linter. * Remove key:string uknown from the base types. * Clean up types after removing key:string unknown. * Lint and prettier fixes. * Implement custom-types Co-authored-by: mdmjg <mdj308@nyu.edu> * added custom types to examples * reset yarn lock * added ts to fixtures * examples custom types * Working fix * ts-thesunny-try * Extract interface types. * Fix minor return type in create-editor. * Fix the typing issue with Location having compile time CustomTypes * Extract types for Transforms. * Update README. * Fix dependency on slate-history in slate-react Co-authored-by: mdmjg <mdj308@nyu.edu> Co-authored-by: Brent Farese <brentfarese@gmail.com> Co-authored-by: Brent Farese <25846953+BrentFarese@users.noreply.github.com> Co-authored-by: Tim Buckley <timothypbuckley@gmail.com>
Answering some questions/addressing concerns: One of the reasons for this unusual way of adding types is so that developers can define custom types once and it should propagate through all of Slate's method calls. If we used generics at call sites, the type has to be passed in to every Slate function call which can be cumbersome. Note that this proposal should be compatible with a generics based version so in the future if somebody wants to contribute that, they can still do so. You could argue that all other things being equal, generics should have been done first; however, another factor is that this was much easier to complete and for many people, some stricter typing was better than nothing. The alternative to this would probably be no custom types. It does appear that the documentation is out of date as you mentioned. That's probably a good reason to keep it in the "@next" release until the docs are improved. We would love contributions to improve this. ❤️ In the repo, you'll find |
Hmm... I'm playing around with the types and it seems like some of the things are broken as the custom types don't appear to be passed through automatically in all the method calls. I will try and sort this out with the other contributors. |
Small note on this:
I agree, and find that the current declarations approach will run into serious limitations in our project, where we have multiple variations on a Slate editor in the same app with different features enabled for different contexts--which runs quickly into difficulties when required to use a global override (as was already acknowledged in the "limitations" above, I realize). EDIT: Adding onto this idea, it seems to me that one of the great advantages of the current Slate is the clean way of extending the editor with |
This post describes a proposal to add custom types to Slate with the following features:
Element
andText
with custom propertiesLimitation:
Simple Schema Example
For simplicity, this is a model for a bare bones version of Slate's types without custom types. I use this to explain the proposal.
The goal:
Element
andText
.getChildren
andgetText
to keep working.Element
andText
in our own code without generics at each call site.How To Use Custom Types
This section explains how to use Custom Types by the end user. I will then explain and show the code for how to add custom types to the schema in the sample above.
Here's a screenshot showing what happens when we uncomment
element.depth
and that the proper typescript error shows up:How It Works
Here is the source code for
./slate.ts
This solution combines
interface
withtype
to get us the best of both.An
interface
supports declaration merging but does not support type discrimination (ie. unions). Atype
supports unions and type discrimination but not declaration merging.The solution uses the declaration merging from
interface
and sets its properties to atype
which can be used in unions and declaration merging.The
ExtendedType
takes two generic arguments. The first argumentK
is the key within theCustomTypes
interface which the end use can extend. The second argumentB
is the base type to use if a custom value is not defined onCustomTypes
and which is being extended.It works by seeing if
CustomTypes[K]
extendsunknown
. If it does, it means that the custom property hasn't been added toCustomTypes
. This is becauseunknown
only extendsunknown
. If this is true (ie. no custom type is provided), we then make the type the Base typeB
(ie.Element
orText
).If it does not extend
unknown
, then a custom type was provided. In that case, we take the custom type atCustomTypes[K]
and add it to the base typeB
.Comments
This solves these typing issues:
The text was updated successfully, but these errors were encountered: