-
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
Custom and Extended Types #3785
Changes from 1 commit
38a0ecc
0312ee5
82c254d
62a8aaa
f1b329f
c6c86e7
14400a5
821b814
1e929c6
4cf3745
e0a9be6
df9c098
0fe3788
304c9cd
052ef78
d086d98
395d91e
cc7fd88
a60e065
18b47f2
d5d816a
329e44e
ccfbead
3b70b09
4f652d9
e55260b
6dc5c3e
8d7e318
7368fd9
82c8f97
d3daf18
de6ba2b
8ee9d1e
f4be265
f250ca6
01204fb
8daeef0
c52308b
e0e7463
d42a3e2
88c15bd
e102403
fcd2bdf
81383bf
bfac456
f8b7568
faa5274
8264419
aa3a235
51ad8ef
dbb2072
d20085f
9c311ae
2a94b1f
95d4d5d
dd2aa6f
37ed24d
ec1e876
43046f4
d91a75a
3f4bcda
c576beb
7dd9ff2
ff2b19f
e328ca7
84448f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,28 +5,23 @@ import { Descendant } from 'slate'; | |
import { Descendant } from 'slate'; | ||
* The `CustomExtensions` interface extends types | ||
*/ | ||
import { Descendant } from './node'; | ||
import { Descendant } from './node' | ||
|
||
export interface BaseElement { | ||
children: Descendant[] | ||
} | ||
|
||
// This would be Text as per Slate's definition | ||
export type BaseText = { | ||
export interface BaseText { | ||
text: string | ||
} | ||
|
||
export interface CustomExtensions { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think we need to update Slate's types for // This would be Element as per Slate's definition
export type BaseElement = {
children: Text[]
}
// This would be Text as per Slate's definition
export type BaseText = {
text: string
}
export type Text = ExtendedType<"Text", BaseText>
export type Element = ExtendedType<"Element", BaseElement> Then, the exported new types for Let's discuss if needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue with this is that when you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I haven't had time to dive into the code so perhaps you could explain the issue and I might be able to help with a solution. My question is, if the original object is defined as BaseText or BaseElement, and the extended type is created as Text and Element, are you saying that the methods don't come through to the extend type? It looks like in the typing we are doing: export type ExtendedType<K extends string, B> =
unknown extends CustomExtensions[K]
? B
: B & CustomExtensions[K] So shouldn't we get the proper types from both the original with the extensions added (ie. I may be misunderstanding the issue in which case a clarification would be helpful. Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thesunny could use a review from your end. I think the code is looking more final now so it's probably a good time to review. Thanks! @CameronAckermanSEL can you review too? |
||
[key: string]: unknown | ||
} | ||
|
||
|
||
// prettier-ignore | ||
export type ExtendedType<K extends string, B> = | ||
unknown extends CustomExtensions[K] | ||
? B | ||
: B | CustomExtensions[K] | ||
|
||
|
||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -222,7 +222,7 @@ export const Node = { | |||||
} | ||||||
} | ||||||
|
||||||
delete r.selection | ||||||
if (Editor.isEditor(r)) delete r.selection | ||||||
cameracker marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
}) | ||||||
|
||||||
return newRoot.children | ||||||
|
@@ -354,8 +354,12 @@ export const Node = { | |||||
|
||||||
matches(node: Node, props: Partial<Node>): boolean { | ||||||
return ( | ||||||
(Element.isElement(node) && Element.isElementProps(props) && Element.matches(node, props)) || | ||||||
(Text.isText(node) && Text.isTextProps(props) && Text.matches(node, props)) | ||||||
(Element.isElement(node) && | ||||||
Element.isElementProps(props) && | ||||||
Element.matches(node, props)) || | ||||||
(Text.isText(node) && | ||||||
Text.isTextProps(props) && | ||||||
Text.matches(node, props)) | ||||||
) | ||||||
}, | ||||||
|
||||||
|
@@ -468,7 +472,7 @@ export const Node = { | |||||
if (Text.isText(node)) { | ||||||
return node.text | ||||||
} else { | ||||||
let children = node.children as Array<Descendant> | ||||||
const children = node.children as Array<Descendant> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tim is totally correct but we should avoid the type assertion, if we can. |
||||||
return children.map(Node.string).join('') | ||||||
} | ||||||
}, | ||||||
|
@@ -477,7 +481,7 @@ export const Node = { | |||||
* Return an iterable of all leaf text nodes in a root node. | ||||||
*/ | ||||||
|
||||||
* texts( | ||||||
*texts( | ||||||
root: Node, | ||||||
options: { | ||||||
from?: Path | ||||||
|
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 should be staying away from type assertions, if at all possible. There is a way to have the same code here w/o the type assertion.