Custom and Extended Types #3785

mdmjg · 2020-07-13T20:24:33Z

Is this adding or improving a feature or fixing a bug?

feature

What's the new behavior?

Uses interfaces to support declaration merging and types for type discrimination.

In order to use declaration merging, the user must augment the Slate module. This detail could be explained in an additional example that could be added for this feature.

Have you checked that...?

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

Does this fix any issues or need any specific reviewers?

Fixes: #3725 #3744
Reviewers: @thesunny @timbuckley @BrentFarese

BrentFarese

See requested changes/comment inline. Thanks.

BrentFarese · 2020-07-16T15:46:31Z

packages/slate/src/interfaces/custom-extensions.ts

+ * The `CustomExtensions` interface extends types
+ */
+
+export interface CustomExtensions {


I still think we need to update Slate's types for Text and Element to use ExtendedType below? See @thesunny suggestion in the implementation issue - #3725. For instance:

// 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 Text and Element would be used in Slate's codebase and any developer's use of CustomExtensions and ExtendedType would propagate to the rest of the codebase.

Let's discuss if needed.

The issue with this is that when you use ExtendedType you are redefining Element or Text according to the definition you gave it inside your CustomExtension (once the user has defined it). If make the extended Element the new Element, a bunch of errors will be thrown during build, and pretty much 60% of the testcases will fail. Something that I found while making this PR was that in order to call Element's functions like isElement, you need to use an alias. If you take a look at my isElement test case, you will see I am calling BaseElement.isElement. If I were to use Element.isElement, the program would not recognize the function because the type Extension is not able to fetch the functions as well.

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. B & CustomExtensions[K])?

I may be misunderstanding the issue in which case a clarification would be helpful. Thanks.

@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?

timbuckley · 2020-07-20T14:35:50Z

packages/slate/src/interfaces/node.ts

@@ -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>


Array<x> is more commonly written as x[]

Suggested change

const children = node.children as Array<Descendant>

const children = node.children as Descendant[]

Tim is totally correct but we should avoid the type assertion, if we can.

BrentFarese · 2020-07-21T13:21:04Z

packages/slate/src/create-editor.ts

@@ -217,7 +217,7 @@ export const createEditor = (): Editor => {
          (editor.isInline(node) ||
            node.children.length === 0 ||
            Text.isText(node.children[0]) ||
-            editor.isInline(node.children[0]))
+            editor.isInline(node.children[0] as Element))


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.

mdmjg · 2020-07-21T19:29:33Z

Note: You will get some errors when you run yarn build because of the placeholder property in the Leaf interface in slate-react. Some help with that would be super appreciated!

BrentFarese · 2020-07-24T17:13:25Z

packages/slate-react/src/components/leaf.tsx

@@ -28,6 +27,8 @@ const Leaf = (props: {
    <String isLast={isLast} leaf={leaf} parent={parent} text={text} />
  )

+  // const placeholder = 'placeholder' in leaf && leaf.placeholder


Do we need this @mdmjg? Let's remove.

BrentFarese · 2020-07-24T17:15:53Z

packages/slate-react/src/components/slate.tsx

@@ -14,7 +14,7 @@ import { EDITOR_TO_ON_CHANGE } from '../utils/weak-maps'

 export const Slate = (props: {
  editor: ReactEditor
-  value: Node[]
+  value: Element[]
  children: React.ReactNode
  onChange: (value: Node[]) => void
  [key: string]: unknown


I think we might be able to get rid of this [key: string]: unknown.

BrentFarese · 2020-07-24T17:16:40Z

packages/slate/src/interfaces/custom-types.ts

@@ -0,0 +1,18 @@
+import { Descendant } from './node'
+
+export interface BaseElement {


I think I would move BaseElement next to the Element type and BaseText next to the Text type (i.e., back in the interface files for those types). Easier to read/follow the source that way.

packages/slate/src/interfaces/text.ts

BrentFarese · 2020-07-24T18:07:15Z

packages/slate/src/transforms/general.ts

@@ -65,7 +65,8 @@ export const GeneralTransforms = {
        if (Text.isText(node) && Text.isText(prev)) {
          prev.text += node.text
        } else if (!Text.isText(node) && !Text.isText(prev)) {
-          prev.children.push(...node.children)
+          const children = node.children as Element[]


I changed the type of editor.children to Descendant[] instead of Element[] and am going to revert some of these changes. We can discuss but I think the more appropriate type for editor.children is Descendant[].

okay! I'll wait for you push then

Co-authored-by: Brent Farese <25846953+BrentFarese@users.noreply.github.com>

…custom-types

lcswillems · 2020-08-17T07:02:49Z

This seems an amazing PR!!

I don't get why @CameronAckermanSEL, @ianstormtaylor and @thesunny should review this PR? They don't seem active. Waiting for their review will just make this PR never get merged and I don't think it is the best thing for the community.

I would suggest that this PR got merged and if there are issues, they will be raised and addressed in a future release, it is not like if the project was currently in a stable state.

Thank you @mdmjg for the awesome work you did in this PR and previous ones (#3762, #3784)!

Co-authored-by: mdmjg <mdj308@nyu.edu>

… into custom-types

cameracker · 2020-08-20T01:40:08Z

packages/slate-react/src/components/editable.tsx

@@ -9,6 +9,7 @@ import {
  Transforms,
  Path,
 } from 'slate'


is this a potential circular dependency?

packages/slate/src/interfaces/node.ts

packages/slate/src/transforms/node.ts

support/fixtures.js

cameracker · 2020-08-20T01:51:45Z

Thank you so much for all the effort put into this PR. This is going to be a great improvement for users who are in typescript.

I had a few small questions about some code that looks like it was changed for reasons perhaps unrelated to this PR. Additionally, there was some code in the fixtures function that was changed that we might want to change back.

The final concern I had was that there was a pretty massive amount of churn in yarn.lock but no corresponding changes to package.json. I think we may want to revert yarn.lock to what it was, unless these changes can be justified. Surprise changes to the lock file are a little bit of a red flag to me.

Thanks again!

BrentFarese · 2020-08-21T20:10:44Z

Closing this in favor of #3835.

mdmjg added 6 commits July 13, 2020 14:22

added custom types

38a0ecc

files

0312ee5

more extensions

82c254d

files

62a8aaa

changed fixtures

f1b329f

changes eslint file

c6c86e7

BrentFarese requested changes Jul 16, 2020

View reviewed changes

mdmjg added 3 commits July 17, 2020 15:46

changed element.children to descendant

14400a5

updated types

821b814

more type changes

1e929c6

timbuckley reviewed Jul 20, 2020

View reviewed changes

mdmjg added 2 commits July 20, 2020 10:28

changed a lot of typing, still getting building errors

4cf3745

extended text type in slate-react

e0a9be6

BrentFarese reviewed Jul 21, 2020

View reviewed changes

removed type assertions

df9c098

BrentFarese and others added 2 commits July 24, 2020 00:27

Clean up of custom types and a couple uneeded comments.

0fe3788

Rename headingElement-true.tsx.tsx to headingElement-true.tsx

304c9cd

BrentFarese reviewed Jul 24, 2020

View reviewed changes

packages/slate/src/interfaces/text.ts Outdated Show resolved Hide resolved

BrentFarese reviewed Jul 24, 2020

View reviewed changes

mdmjg and others added 6 commits July 24, 2020 13:08

moved basetext and baselement

052ef78

Update packages/slate/src/interfaces/text.ts

d086d98

Co-authored-by: Brent Farese <25846953+BrentFarese@users.noreply.github.com>

Fix some type issues with core functions.

395d91e

Merge branch 'custom-types' of github.com:arity-contracts/slate into …

cc7fd88

…custom-types

Clean up text and element files.

a60e065

Convert other types to extended types.

18b47f2

BrentFarese requested a review from cameracker July 24, 2020 19:20

Lint and prettier fixes.

aa3a235

BrentFarese requested a review from ianstormtaylor July 31, 2020 14:12

timbuckley force-pushed the custom-types branch from fd28053 to a634d3d Compare August 11, 2020 19:58

Merge branch 'master' of https://github.com/ianstormtaylor/slate

51ad8ef

timbuckley force-pushed the custom-types branch 2 times, most recently from 889781c to 7a95033 Compare August 11, 2020 20:41

timbuckley and others added 2 commits August 19, 2020 12:38

Merge branch 'master' of https://github.com/ianstormtaylor/slate

dbb2072

Implement custom-types

d20085f

Co-authored-by: mdmjg <mdj308@nyu.edu>

timbuckley force-pushed the custom-types branch from 7a95033 to d20085f Compare August 19, 2020 16:45

mdmjg added 3 commits August 19, 2020 12:51

merge changes

9c311ae

added custom types to examples

2a94b1f

Merge branch 'custom-types' of https://github.com/arity-contracts/slate…

95d4d5d

… into custom-types

cameracker reviewed Aug 20, 2020

View reviewed changes

cameracker requested changes Aug 20, 2020

View reviewed changes

packages/slate/src/interfaces/node.ts Show resolved Hide resolved

packages/slate/src/transforms/node.ts Show resolved Hide resolved

support/fixtures.js Show resolved Hide resolved

mdmjg and others added 11 commits August 20, 2020 08:22

reset yarn lock

dd2aa6f

added ts to fixtures

37ed24d

examples custom types

ec1e876

Set yarn.lock back to match master

43046f4

Fixup Transforms mergine in index

d91a75a

(m) Bring back editor.children Node[] coerc

3f4bcda

(m) Fully bring yarn.lock back to before

c576beb

Fix type coersion on editor.children

7dd9ff2

custom-types.ts -> custom-types.d.ts

ff2b19f

Use thesunny's version of transforms/index

e328ca7

Excluded .ts files from test.

84448f6

BrentFarese closed this Aug 21, 2020

BrentFarese deleted the custom-types branch August 21, 2020 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom and Extended Types #3785

Custom and Extended Types #3785

mdmjg commented Jul 13, 2020 •

edited

Loading

BrentFarese left a comment

BrentFarese Jul 16, 2020 •

edited by timbuckley

Loading

mdmjg Jul 16, 2020 •

edited

Loading

thesunny Jul 16, 2020

BrentFarese Jul 24, 2020

timbuckley Jul 20, 2020

BrentFarese Jul 21, 2020

BrentFarese Jul 21, 2020

mdmjg commented Jul 21, 2020

BrentFarese Jul 24, 2020

mdmjg Jul 24, 2020

BrentFarese Jul 24, 2020

BrentFarese Jul 24, 2020

BrentFarese Jul 24, 2020

mdmjg Jul 24, 2020

lcswillems commented Aug 17, 2020 •

edited

Loading

cameracker Aug 20, 2020

cameracker commented Aug 20, 2020

BrentFarese commented Aug 21, 2020

	const children = node.children as Array<Descendant>
	const children = node.children as Descendant[]

		@@ -0,0 +1,18 @@
		import { Descendant } from './node'

		export interface BaseElement {

Custom and Extended Types #3785

Custom and Extended Types #3785

Conversation

mdmjg commented Jul 13, 2020 • edited Loading

Is this adding or improving a feature or fixing a bug?

What's the new behavior?

Have you checked that...?

Does this fix any issues or need any specific reviewers?

BrentFarese left a comment

Choose a reason for hiding this comment

BrentFarese Jul 16, 2020 • edited by timbuckley Loading

Choose a reason for hiding this comment

mdmjg Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdmjg commented Jul 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcswillems commented Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

cameracker commented Aug 20, 2020

BrentFarese commented Aug 21, 2020

mdmjg commented Jul 13, 2020 •

edited

Loading

BrentFarese Jul 16, 2020 •

edited by timbuckley

Loading

mdmjg Jul 16, 2020 •

edited

Loading

lcswillems commented Aug 17, 2020 •

edited

Loading