Improve typings to add safety for props #51

monodop · 2020-04-23T23:19:28Z

Based on #49, I think I have everything right. Thoughts?

Add tests to prove types are working
Cleanup commits
Split tests to separate PR or Upgrade to TypeScript 3.9.0 TypeScript 3.9 Iteration Plan microsoft/TypeScript#37198

monodop · 2020-04-25T05:21:07Z

I think everything is working now. If you look in src/__tests__/types.tsx you can see all of the typing tests, but they only work in TypeScript 3.9.0, since they rely on the new // @ts-expect-error attribute. If you use this on earlier versions of typescript, you'll just get typescript errors and the tests will fail. Before this gets merged we can probably just comment them out until the new typescript version comes out.

@brainkim are you ok with these changes? I made sure it works with looser typing. If you have your tsconfig set to allow implicit this, you can get even looser and I think everything still works.

brainkim · 2020-04-25T05:27:24Z

@monodop I’ll need to pull down the changes and play around with it. I promise to get you concrete feedback by tomorrow night.

they only work in TypeScript 3.9.0, since they rely on the new // @ts-expect-error attribute

If it’s just for the tests then I don’t mind updating, but I’m typically pretty conservative on adopting new TypeScript features (still haven’t adopted nullish coalescing or optional chaining)

If you have your tsconfig set to allow implicit this

I like to throw any into my code here and there but I always run typescript with --strict

monodop · 2020-04-25T05:32:11Z

Ok, to get the tests running you have to run npm install --save-dev typescript@3.9.0-beta (or whatever the yarn equivalent is). It might be a good idea to wait to put that into the main package until it's actually released though. It's slated for mid-may currently so that's not too long to have those specific tests disabled.

Roadmap: microsoft/TypeScript#37198

If you're not ok with that, I might be able to get dtslint or tsd up and running with some hacky workarounds but my initial attempts at that weren't very successful.

brainkim

Left some thoughts!

src/index.ts

brainkim · 2020-04-25T21:14:49Z

src/index.ts

-const hostNodes = new WeakMap<HostContext<any>, HostNode<any>>();
-export class HostContext<T = any> {
-	constructor(host: HostNode<T>) {
+const hostNodes = new WeakMap<HostContext<any>, HostNode<any, any>>();


Typing props for HostContexts is gonna be tricky, might have to think more about this.

I haven't fully grasped how HostContext works, this is mostly a guess. Is any not sufficient?

src/__tests__/types.tsx

src/index.ts

brainkim

Love the name of the commit “Solved the mystery”😄

src/index.ts

brainkim · 2020-04-30T04:31:59Z

src/index.ts

@@ -691,7 +699,7 @@ class ComponentNode<T> extends ParentNode<T> {
 			return [undefined, undefined];
 		} else if (this.iterator === undefined) {
 			this.ctx.clearEventListeners();
-			const value = new Pledge(() => this.tag.call(this.ctx, this.props!))
+			const value = new Pledge(() => this.tag.call<Context<TProps>, [NonNullable<TProps>], MaybePromise<Child> | ChildIterator>(this.ctx, this.props!))


I was wondering how you managed to invoke Function.prototype.call on the union of GeneratorComponent and FunctionComponent, but I prefer the old way of typing Component?

See microsoft/TypeScript#33815 for why I chose to use a union in the return value versus a union of functions. It‘s just really wonky in TypeScript right now.

I did see that, but I was able to get typescript to be happy by just telling it what I want it to use for the generics. I'd prefer component to be both function and generator, since it can be both, right?

The thing is, generator components are still functions it’s just the return type that’s different. The reason I would want to use the old type is suddenly the union function type can’t be used with call and apply, and at the end of the TypeScript is all about structural typing anyways, so redeclaring types which theoretically have the same structure isn’t a big deal.

In my opinion the types should represent what things are. A component can be a function that returns a child or a promise of a child, or it can be a generator or async generator that yield childs. I get that you can't use .call or .apply on it right now, but that's not really used very many places in the codebase. Are users going to be using .call or .apply? Can we just use hacky workarounds in our own code for now until typescript eventually decides to do something so that it's a nicer experience for the end user?

I mean... the issue was opened last year in October and has yet to receive a comment from any of the TypeScript maintainers. And yes, I do want to leave open the possibility for advanced users to use call or apply on a component type even if they shouldn’t. Who knows maybe there’s an advanced use-case for routers or something.

At the end of the day, FunctionComponent | GeneratorComponent is structurally the same as (this: Context, props: TProps) => ChildIterator | MaybePromiseLike<Child> and I’d prefer the solution which caused fewer hacks, given that TypeScript is structurally typed.

You know what? I just realized I was arguing about the wrong thing. With all the changes I was doing while testing stuff, I forgot what the original typing was and I think they're both pretty much the same thing. For some reason I thought the ChildIterator part wasn't in the original typing. So yeah I think you're right and I'll change it back.

monodop · 2020-04-30T04:43:09Z

Love the name of the commit “Solved the mystery”😄

Yeah I felt stupid after I figured out what I was doing wrong. I'll squash most of these commits once everything's settled down a bit so the history is a little cleaner.

brainkim

Overall I’m very impressed by the work. Thank you so much for doing this stuff. I did some naughty things and moved around some methods and files around when trying to hotfix some showstopper bugs (#78) but I don’t expect you to rebase around it. I can pull it down and make sure your code is properly attributed. In regards to squashing commits. I don’t really care. I like it when commit messages are expressive and show some of the sweat and labor people put into writing code and you can go back and go hey remember when I was stuck on this.

src/index.ts

brainkim · 2020-04-30T04:36:48Z

src/__tests__/types.tsx

+        // Not entirely sure why this one doesn't error anymore. seems that <a></a> returns
+        // any, while createElement('a') returns Element<any>.
+        // skip @ts-expect-error
+        // const MyElement: Component = <a></a>;


We can specify the type of a JSX element by defining the JSX.Element on the global JSX type. I think. I’m not 100% sure but that’s what the docs say. https://www.typescriptlang.org/docs/handbook/jsx.html#the-jsx-result-type

I'll check it out

I spent about 15 minutes playing with this, and it seems super complicated. I think JSX.Element needs to be typed as MaybePromise<Child>, and anywhere you used Element in the tests should really be MaybePromise<Child> or Child. renderer.render will need to accept MaybePromise<Child> too.

I think the types on ChildIterator are wrong, should probably be Iterator<Child, Child | void, TNext> and AsyncIterator<Child, Child | void, TNext>.

It gets really ugly because you have to use MaybePromise<Child> a lot in the tests. I tried wrapping Child with MaybePromise so that it's always included, but then async components break because they are not MaybePromise<Child>, they're actually Promise<Child>.

This change will definitely break a lot of existing code that rely on Element. Might be worth making a separate PR for it.

Edit: I could be wrong though, this is just my first impressions. I'll keep looking! Can also provide some screenshots of errors and stuff if you want. But I suggest you just try adding the JSX.Element type and see all the errors light up 🙂

Doesn’t have to be part of the PR if you’re eager to get these changes shipped.

brainkim · 2020-04-30T04:40:40Z

src/__tests__/types.tsx

@@ -0,0 +1,249 @@
+/** @jsx createElement */


I really really love these type tests. The one thing I’m worried about is that the jest linter will possibly complain about it because none of these tests have actual assertions. Also we should probably just upgrade to TypeScript 3.9 RC in package.json if we’re going to be using these features (https://devblogs.microsoft.com/typescript/announcing-typescript-3-9-rc/)

https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/expect-expect.md this is the rule that I expect to start throwing. Maybe we can disable it for the file.

I haven't seen any linter errors when I run npm run test, but maybe I'm missing something. I'll update package.json with the rc version - was just hesitant to do so unless you thought it was a good idea. I don't know if there's any implications for users if we do this though.

We’re using yarn so it would be yarn run test. But anyways the linting stuff is yarn run lint and you can fix prettier errors automatically with yarn run lint --fix.

We definitely need a CONTRIBUTING.MD at some point.

Ah ok. I've actually never used yarn before so I'll look into it. Would be nice to get that all automated in github at some point 😄

src/__tests__/index.tsx

src/index.ts

monodop · 2020-04-30T05:09:04Z

I have no issues with rebasing and rewriting commits for this. I do it all the time at my job and I think it really keeps things looking nice (personally I hate seeing "fix typo", "oopsies!" commits that could just be one thing 🙂). Usually I try to preserve smaller logical commits if there's atomic changes that make up the full PR. I just don't do it until the end because it makes it harder to review if everything's being rebased all the time. I think you'll be happy with it :)

brainkim · 2020-04-30T05:11:04Z

If you’re happy to rebase and squash I’m happy. But know that I will gladly help if the rebase turns out to be hairy (we both touched index.ts a little bit).

src/index.ts

monodop · 2020-04-30T08:16:40Z

Ok I think this might be ready to go. I noticed I updated some of the types from MaybePromiseLike to MaybePromise. Do you think this will cause any issues? I'm not too sure what the practical difference is.

brainkim · 2020-04-30T17:54:52Z

@monodop I prefer to have components return promise-likes rather than promises. Promise-likes means something which implements the then method as specified by the promises/A+ spec. Promises are the actual ES6 promise class, which includes the methods catch and finally. Even if most TypeScript people are using regular promises, I want Crank to follow the robustness principle and accept promise-likes even if we work output promises exclusively. Typing the return value as promise-likes means we’re forced to consider the possibility that components return promise-likes in internal code.

monodop · 2020-04-30T18:18:10Z

Okay, I put the MaybePromiseLike's back. Anything else?

brainkim · 2020-04-30T18:35:22Z

@monodop I need to think a little more about the way props work as they flow into the hidden ParentNode classes. For instance, you set the ParentNode props to the Props of the current node, which I think is incorrect. It implies that every node of the element tree has the same props shape, which isn’t true. I also need to think a little about HostNode props, and how Props relate to the IntrinsicProps type. Finally, I’m also considering dropping the requirement of having children be of the Children type. I’ve seen Crank code in the wild which uses render props for children, and while I hate the pattern, I don‘t want to prevent it with TypeScript.

I’m trying not to be a finicky code reviewer, sorry 😖.

monodop · 2020-04-30T19:01:52Z

No worries, I'm fine with iterating on this until it's right. I felt a little weird typing out all the node elements, but it at least it all compiles at this point. I found the naming for a lot of these things to be kind of confusing so I wasn't exactly sure how the types needed to be set. I'm not sure what all the node types are supposed to be, since they all extend ParentNode, but really they should just be regular Nodes, right?

monodop · 2020-04-30T21:43:51Z

What do you think about the changes in b6ba714

package.json

brainkim · 2020-05-05T20:52:53Z

Closed for #96
Most work was preserved, and just added a single commit which tweaks some stuff and adds some tests for the new createElement typings:
22cac8a

Thanks for working on this stuff @monodop!

monodop · 2020-05-05T23:18:04Z

Oh hey sorry I didn't get back to you on this one. Thanks for cleaning it up and getting it in!

brainkim · 2020-05-05T23:24:03Z

@monodop Sorry I messed up the merge! And again, super impressed with the TS-foo. TagProps seems to work perfectly!

monodop · 2020-05-05T23:27:49Z

Why did you untype context from ComponentNode?

brainkim · 2020-05-05T23:30:39Z

@monodop line number?

monodop · 2020-05-05T23:31:15Z

here

brainkim · 2020-05-05T23:42:14Z

Ahhhh hmmm I think I was removing TProps stuff from parent node and I had a type error with ComponentNode and inheritance, but it probably could be more strictly typed there. Good catch, my bad.

monodop · 2020-05-05T23:46:13Z

I also thought it was odd to untype Props from tag as well from ComponentNode.

monodop · 2020-05-05T23:47:15Z

If there's anything I can do to help fix any weird type errors let me know and I can take a stab at it. Maybe needs a new ticket or something though 🙂

brainkim · 2020-05-05T23:57:48Z

I also thought it was odd to untype Props from tag as well from ComponentNode

If you’re asking why we don’t use the Props type from being used it’s mostly because I didn’t want to prevent render props as children via types, which is what that type would do if someone writes a render prop and then iterates over this.

brainkim · 2020-05-05T23:59:06Z

@monodop Definitely! I’m always interested in better typings and if you ever want to get a deeper understanding of the admittedly intense async code which I haven’t commented yet, I’m down to give a walkthrough. Might do a twitch or discord stream at some point with people interested to lower bus factor of the code.

monodop · 2020-05-06T00:34:16Z

I also thought it was odd to untype Props from tag as well from ComponentNode

If you’re asking why we don’t use the Props type from being used it’s mostly because I didn’t want to prevent render props as children via types, which is what that type would do if someone writes a render prop and then iterates over this.

Nope, was more asking about why you changed tag from Tag<TProps> to Tag (effectively Tag<any>)

monodop · 2020-05-06T00:36:19Z

@monodop Definitely! I’m always interested in better typings and if you ever want to get a deeper understanding of the admittedly intense async code which I haven’t commented yet, I’m down to give a walkthrough. Might do a twitch or discord stream at some point with people interested to lower bus factor of the code.

I'd be super interested depending on the time difference. Is there already a discord or some other chat we can use for this project? This in particular is getting a little off topic but I'd like to continue the discussion

brainkim · 2020-05-06T00:38:36Z

@monodop Ahhh if you’re asking about the tag value in ParentNode, then it’s because I removed the TProps type parameter from the ParentNode class. I do want to type it at some point it just gives me a headache to think about right now how the TProps parameter would work with inheritance, and I’m not really sure about the value of typing it on the internal nodes yet. What was important for the PR was that Component and Context were properly typed for dependents and we can figure out the nitty gritty about how TProps flow through the internal node classes in another PR I guess.

monodop · 2020-05-06T00:40:52Z

@monodop Ahhh if you’re asking about the tag value in ParentNode, then it’s because I removed the TProps type parameter from the ParentNode class. I do want to type it at some point it just gives me a headache to think about right now how the TProps parameter would work with inheritance, and I’m not really sure about the value of typing it on the internal nodes yet. What was important for the PR was that Component and Context were properly typed for dependents and we can figure out the nitty gritty about how TProps flow through the internal node classes in another PR I guess.

Ok, makes sense. Having type safety on the component signature was my biggest goal here, and I think we've got that nailed down. Was just curious about the details 👍

brainkim · 2020-05-06T00:42:08Z

No discord yet. Need to work on community stuff but right now I’m just trying to build on the momentum of the initial release. Lots of work to do and happy for all the help! I’m on discord at brain#4221 if you want to contact me directly though!

ryhinchey · 2020-05-06T01:19:55Z

happy to help with community stuff eventually

monodop mentioned this pull request Apr 24, 2020

Typescript support for props in Component type #49

Closed

monodop force-pushed the feature/type-safe-props branch from ec88caa to c15e90e Compare April 25, 2020 05:15

brainkim self-requested a review April 25, 2020 20:15

brainkim reviewed Apr 25, 2020

View reviewed changes

codejet reviewed Apr 26, 2020

View reviewed changes

src/index.ts Show resolved Hide resolved

brainkim self-requested a review April 26, 2020 22:57

monodop force-pushed the feature/type-safe-props branch from aa8b4e2 to 7f1a6ea Compare April 27, 2020 17:23

brainkim reviewed Apr 30, 2020

View reviewed changes

src/index.ts Show resolved Hide resolved

brainkim reviewed Apr 30, 2020

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

monodop force-pushed the feature/type-safe-props branch from 5afefd7 to 84714b7 Compare April 30, 2020 08:10

monodop requested a review from brainkim April 30, 2020 08:16

monodop added 4 commits April 30, 2020 11:17

Improve typings to add safety for props

f577c48

Add support for Intrinsic Elements from TagProps

edcce48

Upgrade to TypeScript 3.9.1-rc

693ec40

Add typechecking test cases

79cc371

monodop force-pushed the feature/type-safe-props branch from 84714b7 to 79cc371 Compare April 30, 2020 18:17

wip: remove prop typing from node's parent attribute

b6ba714

Merge branch 'master' into feature/type-safe-props

69e6662

brainkim reviewed May 4, 2020

View reviewed changes

package.json Show resolved Hide resolved

brainkim mentioned this pull request May 5, 2020

Monodop/feature/type safe props #96

Merged

brainkim closed this May 5, 2020

Improve typings to add safety for props #51

Improve typings to add safety for props #51

Conversation

monodop commented Apr 23, 2020 • edited Loading

monodop commented Apr 25, 2020 • edited Loading

brainkim commented Apr 25, 2020

monodop commented Apr 25, 2020

brainkim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brainkim left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monodop Apr 30, 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

monodop commented Apr 30, 2020

brainkim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monodop Apr 30, 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

brainkim Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monodop commented Apr 30, 2020 • edited Loading

brainkim commented Apr 30, 2020

monodop commented Apr 30, 2020

brainkim commented Apr 30, 2020 • edited Loading

monodop commented Apr 30, 2020

brainkim commented Apr 30, 2020 • edited Loading

monodop commented Apr 30, 2020

monodop commented Apr 30, 2020

brainkim commented May 5, 2020 • edited Loading

monodop commented May 5, 2020

brainkim commented May 5, 2020

monodop commented May 5, 2020

brainkim commented May 5, 2020

monodop commented May 5, 2020 • edited Loading

brainkim commented May 5, 2020

monodop commented May 5, 2020

monodop commented May 5, 2020

brainkim commented May 5, 2020

brainkim commented May 5, 2020

monodop commented May 6, 2020

monodop commented May 6, 2020

brainkim commented May 6, 2020 • edited Loading

monodop commented May 6, 2020

brainkim commented May 6, 2020

ryhinchey commented May 6, 2020

monodop commented Apr 23, 2020 •

edited

Loading

monodop commented Apr 25, 2020 •

edited

Loading

brainkim left a comment •

edited

Loading

monodop Apr 30, 2020 •

edited

Loading

monodop Apr 30, 2020 •

edited

Loading

brainkim Apr 30, 2020 •

edited

Loading

monodop commented Apr 30, 2020 •

edited

Loading

brainkim commented Apr 30, 2020 •

edited

Loading

brainkim commented Apr 30, 2020 •

edited

Loading

brainkim commented May 5, 2020 •

edited

Loading

monodop commented May 5, 2020 •

edited

Loading

brainkim commented May 6, 2020 •

edited

Loading