Add typescript support to issue-tracker #122

renanmav · 2019-11-12T23:27:39Z

As talked with @jstejada, @josephsavona and @ksaldana1, we would like to add typescript support to issue-tracker example.

And here it is.

Just a minor problem. When we add the Concurrent mode like this, the Root.tsx component with header isn't rendered anymore. To be honest, I don't know why this is happening. Any help?

facebook-github-bot · 2019-11-12T23:27:50Z

Hi renanmav! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

josephsavona

Thanks so much for working on this! I think this could really help folks understand the code. The overall theme of my feedback is to keep the changes focused on adding TypeScript support: please revert arbitrary naming, content, and formatting changes.

issue-tracker/src/Root.tsx

issue-tracker/src/routing/Link.tsx

issue-tracker/src/routing/RouteRenderer.tsx

issue-tracker/src/IssueListItem.tsx

issue-tracker/src/IssueDetailRoot.tsx

issue-tracker/src/IssueDetailComments.tsx

issue-tracker/src/IssueActions.tsx

issue-tracker/.eslintrc

issue-tracker/.prettierrc

…s & returns

It should be next to RouteComponent, which is the component that uses this type

issue-tracker/src/JSResource.ts

josephsavona

Awesome, thanks for the updates! There's still a few things around the types and extraneous changes - also looks your editor really wants to automatically "correct" the capitalization in "GitHub" for some reason!

issue-tracker/src/IssueActions.tsx

issue-tracker/src/IssueDetailComments.tsx

issue-tracker/src/IssueDetailRoot.tsx

issue-tracker/src/Issues.tsx

issue-tracker/src/JSResource.ts

josephsavona · 2019-11-14T14:19:08Z

issue-tracker/src/Root.tsx


-export default function Root(props) {
+const Root = (props: Props) => {


just curious, why the change here?

Tangentially related—usually when declaring functional components in TypeScript, I tend to use const declarations with a React.FC<Props> generic type annotation. This both enforces the return type of your function, as well as correctly typing the Props interface. This current definition is actually a bit misleading—with the way this is declared you will not have a children prop. This may not matter now, but it becomes annoying the second you want to add it. Using React.FC will add those additional properties for you on top of your generic argument.

const Root: React.FC<Props> = (props) => {} // props is typed with our generic + children

Checkout tip 20.
https://medium.com/@martin_hotell/10-typescript-pro-tips-patterns-with-or-without-react-5799488d6680

I've also come across that medium article once, and even if some of the tips are useful, I'd still go with the official way of typing react functional components, like most people are used to, with React.FC<Props> just like @ksaldana1 suggested.

issue-tracker/src/routing/RouteRenderer.tsx

issue-tracker/src/routing/createRouter.ts

issue-tracker/config-overrides.js

issue-tracker/src/RelayEnvironment.ts

issue-tracker/src/routes.ts

issue-tracker/src/routing/RouteRenderer.tsx

renanmav · 2019-11-15T23:51:22Z

I'm sorry about all the unnecessary changes. This was a study that became this PR.

josephsavona

So close!

issue-tracker/src/routing/RouteRenderer.tsx

issue-tracker/src/routes.ts

issue-tracker/src/routing/Link.tsx

sorenhoyer · 2019-11-20T09:33:36Z

issue-tracker/src/routing/createRouter.ts

+  exact?: boolean;
+  strict?: boolean;
+  component: Resource<any>;
+  prepare: (params: {


I know it's not dead necessary for this example, but how about making prepare optional, so we can use the router for non-relay routes also?

I agree with you, but a comment made by @josephsavona:

this could be non-optional

sorenhoyer · 2019-11-20T11:58:24Z

issue-tracker/src/routing/Link.tsx


 /**
 * An alternative to react-router's Link component that works with
 * our custom RoutingContext.
 */
-export default function Link(props) {
+const Link: React.FC<Props> = props => {


Any reason for not destructuring the props eg. const Link: React.FC<Props> = ({ children, to }) => {}? Same with all other instaces of React.FC

Also children is already defined by the FC type, so no reason to define it again in Props interface.

These destructures are really necessary? I think that they add an unnecessary complexity.

no reason to define it again

Thanks!

Isn't destructing props in functional components the standard? How is this complexity? If you did that you could remove 8 unnecessary props. just in that component.

Yeah, but I would add 8 unnecessary new lines to destructure. I'd have 16 lines between destructuring and usage against 8 lines with actual strategy.

In fact, using props.<propName> enforces that this data comes as props.

So... I guess this is pretty basic stuff but...
Changing const Link: React.FC<Props> = props => {} to const Link: React.FC<Props> = ({ children, to }) => {} means that, in just that Link component, you can remove props. from 7 instances of props.to and 1 instance of props. from props.children... how is this adding complexity to you... or 16 new lines. If anything it reduces redundancy (or what am I not seeing here?)

Sorry, what you say makes no sense to me. Thought everyone was doing it since ES2015. Maybe read this.

…mples into feat/issue-tracker-ts

renanmav · 2019-11-25T14:06:27Z

I think we are ready @josephsavona @jstejada

josephsavona · 2019-11-25T14:56:58Z

@renanmav Awesome, thank you again for your contributions. I'll check this out locally and experiment a bit and may make a few minor tweaks before landing, but this looks great.

EDIT: With the combination of holidays and some personal/family issues I didn't get a chance to follow-up on this, my apologies.

PatoBeltran · 2019-12-13T22:51:09Z

I'm interested in seeing this working too, let me know if there's any work to be done so I can jump in

jgcmarins · 2020-03-19T19:06:00Z

What's missing for this one to be merged?

daig · 2020-03-27T03:49:04Z

Ahhhh everything in this repo seems abandoned :/

jgcmarins · 2020-03-27T09:18:23Z

Hi @daig,
We've been using the same code from issue-tracker in production with suspense and preloadQuery and everything works just fine. Nothing abandoned here.
You can follow @sibelius Live for React Europe as well. It follows the exact same implementation used by issue-tracker.

josephsavona · 2020-03-27T13:07:20Z

Just noting that we haven't added many changes here because the example is flushed out and complete. We fully intend to keep this example up-to-date as our APIs evolve, but so far that hasn't warranted any changes because these APIs are now fairly stable.

TypeScript support - this PR - would certainly be nice to have, but I already spent a lot of time reviewing this and simply ran out of steam. This PR is probably going to have to wait until I or someone else on the team have enough free personal time to review again, make edits, and land.

sorenhoyer · 2020-03-27T21:16:49Z

There are still many things that could be improved so good that you did not merge it yet. It makes somewhat sense to make this pr overwrite the existing js code during development to document the changes/diffs. But may I suggest to keep both a js and ts version by making one final commit where the folder is renamed to something like issue-tracker-ts or moved to a ts and then bring back the js version - after this PR is in a good state and approved of course.

sibelius · 2020-03-27T21:43:49Z

what about keep here Flow examples, and create https://github.com/relay-tools/relay-examples-typescript?

so we can move faster and iterate more there, as relay team is not using typescript

josephsavona · 2020-03-29T01:15:34Z

A separate repo for the TS version makes a lot of sense. Please post back here as you make progress, so that we can take a look and provide feedback, and also add a link to it!

add typescript support to issue-tracker

e3bfb66

josephsavona requested changes Nov 13, 2019

View reviewed changes

jstejada reviewed Nov 13, 2019

View reviewed changes

issue-tracker/.prettierrc Outdated Show resolved Hide resolved

renanmav added 7 commits November 13, 2019 13:11

revert prettier config

ec59071

replace non-null assertions for optional chaining on Root

02ae738

throw if router doesnt exists on Link and remove unnecessary chaining…

a8424c6

…s & returns

move RouteComponentProps to RouteRenderer

f694d38

It should be next to RouteComponent, which is the component that uses this type

avoid braceless

9abf9fb

update comment about @ts-ignore on RelayEnvironment.ts

32c099d

make Resource more precise

bdb47e0

renanmav commented Nov 13, 2019

View reviewed changes

issue-tracker/src/JSResource.ts Outdated Show resolved Hide resolved

renanmav added 4 commits November 13, 2019 19:15

revert IssueListItem to IssuesListItem

97007ce

revert README changes

e2098e2

revert more README changes

2c6a186

revert e to event on IssueActions

d8411c5

josephsavona requested changes Nov 14, 2019

View reviewed changes

josephsavona reviewed Nov 14, 2019

View reviewed changes

issue-tracker/config-overrides.js Outdated Show resolved Hide resolved

sorenhoyer reviewed Nov 14, 2019

View reviewed changes

issue-tracker/src/RelayEnvironment.ts Outdated Show resolved Hide resolved

sorenhoyer reviewed Nov 14, 2019

View reviewed changes

issue-tracker/src/routes.ts Outdated Show resolved Hide resolved

force rename of GitHub

0285f0f

sorenhoyer reviewed Nov 14, 2019

View reviewed changes

issue-tracker/src/routing/RouteRenderer.tsx Outdated Show resolved Hide resolved

renanmav added 8 commits November 14, 2019 20:59

return Suspense with null fallback inside SuspenseList

dd2786c

rename fragment IssueListItem to IssuesListItem

4988da4

make result either T of obj with default

927e2eb

revert disable button on IssueActions

f6547ee

revert the arrow function on Root

706cd71

revert concat on RouteRenderer

ab0b7ea

make prepare a mandatory function

58e89a3

use FetchFunction instead of typing function args

263a82e

josephsavona reviewed Nov 18, 2019

View reviewed changes

issue-tracker/src/routing/RouteRenderer.tsx Outdated Show resolved Hide resolved

issue-tracker/src/routes.ts Outdated Show resolved Hide resolved

renanmav added 8 commits November 18, 2019 20:19

throw if there isnt a router on RouteRenderer

a0d4660

remove unnecessary assertions on router

d621941

add type guardian on JSResource

871bdca

make RouteConfig entry general

6836fbd

revert changes on README

af5b394

revert changes on README

7ddec0c

use React.FC

01baff6

add macro config to avoid false positive errors on yarn start

79e1a5e

sorenhoyer reviewed Nov 20, 2019

View reviewed changes

issue-tracker/src/routing/Link.tsx Outdated Show resolved Hide resolved

sorenhoyer reviewed Nov 20, 2019

View reviewed changes

renanmav added 4 commits November 20, 2019 07:02

make all react components as React.FC

aa5def7

Merge branch 'master' into feat/issue-tracker-ts

7cd9538

Update README.md

cc1c367

Update .prettierrc

5ba1807

sorenhoyer reviewed Nov 20, 2019

View reviewed changes

renanmav mentioned this pull request Nov 20, 2019

Propostal to use relay experimental entria/entria-fullstack#118

Merged

renanmav added 2 commits November 20, 2019 19:16

remove unnecessary children prop

71ce2bf

Merge branch 'feat/issue-tracker-ts' of github.com:renanmav/relay-exa…

3d8ba57

…mples into feat/issue-tracker-ts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typescript support to issue-tracker #122

Add typescript support to issue-tracker #122

renanmav commented Nov 12, 2019 •

edited

facebook-github-bot commented Nov 12, 2019

josephsavona left a comment

josephsavona left a comment

josephsavona Nov 14, 2019

ksaldana1 Nov 14, 2019 •

edited

renanmav Nov 14, 2019

sorenhoyer Nov 14, 2019

renanmav commented Nov 15, 2019

josephsavona left a comment

sorenhoyer Nov 20, 2019

renanmav Nov 20, 2019

sorenhoyer Nov 20, 2019 •

edited

renanmav Nov 20, 2019

sorenhoyer Nov 21, 2019

renanmav Nov 21, 2019

sorenhoyer Nov 21, 2019

renanmav commented Nov 25, 2019

josephsavona commented Nov 25, 2019 •

edited

PatoBeltran commented Dec 13, 2019

jgcmarins commented Mar 19, 2020

daig commented Mar 27, 2020

jgcmarins commented Mar 27, 2020

josephsavona commented Mar 27, 2020 •

edited

sorenhoyer commented Mar 27, 2020 •

edited

sibelius commented Mar 27, 2020

josephsavona commented Mar 29, 2020


		export default function Root(props) {
		const Root = (props: Props) => {

Add typescript support to issue-tracker #122

Are you sure you want to change the base?

Add typescript support to issue-tracker #122

Conversation

renanmav commented Nov 12, 2019 • edited

facebook-github-bot commented Nov 12, 2019

josephsavona left a comment

Choose a reason for hiding this comment

josephsavona left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksaldana1 Nov 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renanmav commented Nov 15, 2019

josephsavona left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenhoyer Nov 20, 2019 • edited

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

renanmav commented Nov 25, 2019

josephsavona commented Nov 25, 2019 • edited

PatoBeltran commented Dec 13, 2019

jgcmarins commented Mar 19, 2020

daig commented Mar 27, 2020

jgcmarins commented Mar 27, 2020

josephsavona commented Mar 27, 2020 • edited

sorenhoyer commented Mar 27, 2020 • edited

sibelius commented Mar 27, 2020

josephsavona commented Mar 29, 2020

renanmav commented Nov 12, 2019 •

edited

ksaldana1 Nov 14, 2019 •

edited

sorenhoyer Nov 20, 2019 •

edited

josephsavona commented Nov 25, 2019 •

edited

josephsavona commented Mar 27, 2020 •

edited

sorenhoyer commented Mar 27, 2020 •

edited