[Relay] Uses latest version and our patches #982

alloy · 2018-02-06T21:27:24Z

Uses our custom Node ID patch
Uses our language plugin patch
Uses the TS language plugin I’ve been working on with @kastermester 🙌

I decided to start checking in the artefacts, because now that we have and use TS typings it will be useful to be able to see those when reviewing PRs.

⚠️ If you already had Emission checked out before, be sure to delete all previous artefacts and reset the transformer cache:

$ find . -name __generated__ | xargs rm -rf
$ yarn relay
$ yarn start-packager --reset-cache

Afterwards you can use the regular script again:

$ yarn start

#skip_new_tests

alloy · 2018-02-06T21:32:58Z

Seems to fail on CI due to ds300/patch-package#36. Will look into that tomorrow; for those interested, you should be able to pull and run this locally.

alloy · 2018-02-06T21:55:18Z

src/lib/Scenes/Home/Components/ForYou/Components/ArtworkCarouselHeader.tsx

+      #        i.e. in this case the artist selection is not present in the second fragment.
+      #
+      #        This can be seen much more clear when adding __typename to the context part in ArtworkRail.tsx.
+      #


Working around a bug here ^

ArtsyOpenSource · 2018-02-07T10:42:01Z

	Warnings
⚠️	Missing Test Files: `src/lib/Components/Artist/Articles/__tests__/index-tests.tsx` `src/lib/Components/Artist/Artworks/__tests__/index-tests.tsx` `src/lib/Components/Artist/Shows/__tests__/index-tests.tsx` `src/lib/Components/ArtworkGrids/__tests__/GenericGrid-tests.tsx` `src/lib/Components/ArtworkGrids/RelayConnections/__tests__/ArtistForSaleArtworksGrid-tests.tsx` `src/lib/Components/ArtworkGrids/RelayConnections/__tests__/ArtistNotForSaleArtworksGrid-tests.tsx` `src/lib/Components/Home/ArtistRails/__tests__/ArtistRail-tests.tsx` `src/lib/Components/Home/ArtworkRails/__tests__/ArtworkRailHeader-tests.tsx` `src/lib/Components/Home/__tests__/HeroUnits-tests.tsx` `src/lib/Components/Inbox/ActiveBids/__tests__/index-tests.tsx` `src/lib/Components/Inbox/Conversations/Preview/Attachment/__tests__/AttachmentPreview-tests.tsx` `src/lib/Components/Inbox/Conversations/__tests__/index-tests.tsx` `src/lib/Components/RelatedArtists/__tests__/RelatedArtist-tests.tsx` `src/lib/Components/RelatedArtists/__tests__/index-tests.tsx` `src/lib/Scenes/Favorites/Components/Artists/__tests__/index-tests.tsx` `src/lib/Scenes/Favorites/Components/Artworks/Relay/__tests__/ArtworksRenderer-tests.tsx` `src/lib/Scenes/Favorites/Components/Artworks/__tests__/index-tests.tsx` `src/lib/Scenes/Favorites/Components/Categories/__tests__/index-tests.tsx` `src/lib/Scenes/Home/Components/ForYou/Components/__tests__/ArtworkCarousel-tests.tsx` `src/lib/Scenes/Home/Components/ForYou/Components/__tests__/ArtworkCarouselHeader-tests.tsx` `src/lib/Scenes/Home/Components/ForYou/__tests__/index-tests.tsx` `src/lib/Scenes/Home/Components/Sales/Relay/__tests__/SalesRenderer-tests.tsx` If these files are supposed to not exist, please update your PR body to include "#skip_new_tests".

Generated by 🚫 dangerJS

kastermester · 2018-02-07T10:50:41Z

I hope this works out well for you guys - and I look forward to continue working with @alloy to make sure TS and Relay feels like tools that are made to work together! :)

alloy · 2018-02-07T11:16:22Z

Alright, this is all green now 👍

orta

Yep cool, looks 👍

orta · 2018-02-07T13:16:32Z

.gitattributes

@@ -1 +1,2 @@
 CHANGELOG.md merge=union
+src/__generated__/*.graphql.ts linguist-generated


Good stuff, thanks @pchaigno 🥇

@orta - can you explain whats going on here?

I can - here's the historical context: https://twitter.com/pchaigno/status/961134607113637889

tyty, makes sense 👍

orta · 2018-02-07T13:16:51Z

.prettierignore

@@ -0,0 +1,2 @@
+src/__generated__/*.ts
+node_modules


also, a great move 👍

Turns out that the graphql-js patch wasn’t applying because the whole file had changed due to prettier on save :)

orta · 2018-02-07T13:18:26Z

src/__generated__/About_artist.graphql.ts

@@ -0,0 +1,106 @@
+/* tslint:disable */


Ah, shame, guess the gitattributes has to be merged before we get to use it in PRs

What do you mean? Is it not working for you?

They're flattened, yep, but I can't tell if that's because the PR is too long because all of the in app source code changes are also flattened after it.

Ah I see. I’m pretty sure that some of these artefacts were expanded by default yesterday.

orta · 2018-02-07T13:20:53Z

src/__generated__/About_artist.graphql.ts

+import { ConcreteFragment } from "relay-runtime";
+export type About_artist = {
+    readonly has_metadata: boolean | null;
+    readonly is_display_auction_link: boolean | null;


In our schema this looks like:

type ArtistItem implements Node { [...] # Only specific Artists should show a link to auction results. is_display_auction_link: Boolean }

It would be nice to bring these comments into the interface in v1.1 of the TS generator

Great idea!

Great idea, I have some experience with this in some internal tooling we use where I work. Can you create an issue in https://github.com/relay-tools/relay-compiler-language-typescript ?

relay-tools/relay-compiler-language-typescript#35

Ah great, I threw a few minutes at understanding the code to add it- happy to leave that to you 🍡

Unless @kastermester has time immediately, I think that somebody else –such as yourself– contributing it would be great. At least you’ll know who to ask questions and perhaps @kastermester knows of some things to keep in mind from the top of his head.

Ergh, that alloy, always pushing for more contributors 📟

Will continue my digging

The biggest concern here comes from interface types and how they interact with the type generation (without remembering exactly how that works in the TS plugin here mind you). If the type ends up being something like:

type MyType = { __typename: 'MyType1'; name: string; } | { __typename: 'MyType2'; name: string; } | { // Some comment here telling that this is to cover future types being added __typename: ' %other'; }

The question now becomes - which comment should be added to the name property - assuming there's one for the interface, and one for each of the types. Previously I have added the comment from the concrete types, such that the interface comment never shows up, but simply the concrete type comments. This has the effect on editors (at least VS Code but AFAIk all editors get their info from the TS language server) that you get the combined comment when simply accessing name.

I would love to find a better way to handle this sort of thing - but disregarding that, it should be pretty straight forward. If you have the time to make a pull request that'd be great.

@orta FYI graphql/graphql-js#1165

orta · 2018-02-07T13:23:58Z

src/lib/Containers/Artist.tsx

      case TABS.SHOWS:
-        return <Shows artist={this.props.artist} />
+        return <Shows artist={this.props.artist as any} />


I assume these additional as any are due to the relay data masking, I think @kastermester covers that here

Btw, the only reason that in some cases we didn’t need to specify as any previously was that those components were overriding the constructor without re-typing the props, which meant that the component basically accepted anything by default.

The future plans tie in with the way the artifacts are generated - it means that you cannot pass data to a component, without having actually specified the fragment as part of your fragment text (ie. you need to spread the child component's fragment into your own at the appropriate place to be able to render the child component with the given data).

As mentioned, we're waiting for conditional types coming in TS 2.8 (and conditional infered types as well to be able to type up the componentRef prop).

orta · 2018-02-07T13:24:57Z

src/lib/Containers/Inbox.tsx

-      this.activeBids.refs.component.refreshActiveBids()
-      this.conversations.refs.component.refreshConversations(() => {
+      this.activeBids.refreshActiveBids()
+      this.conversations.refreshConversations(() => {


nice, simpler

And using public API.

Just nitpicking for you guys here: Stuff coming from ref callbacks in React can be set to null (by the ref callback) ie. when the component is unmounting. I'd advice setting the type to T | null and then initialize it to null. Also newer TS versions have definite assignment checks that will complain about this sort of code.

You can then define a method such as this:

function assertNonNull<T>(obj: T | null): T { if (obj == null) { throw new Error('Object is null'); } return obj; }

And use that when you assume the object is non null. You may be aware of this and simply having chosen to save the hassle, but I thought I'd let you know :)

Nice one 👌 I actually was thinking of using invariant soon, which I assume does this type narrowing too, so that we can use existing babel transforms to strip those assertions out for production code.

Regarding the ref callback also calling for unmounting, that’s just a thing that never happens for these two components.

I'm not sure you can type a function to never return if some condition is not met, sadly. I believe it is baked into flow that the invariant function behaves like that.

Regarding the ref callback also calling for unmounting, that’s just a thing that never happens for these two components.

Yeah I know, I'm just personally a bit obsessed with semantics and there's something in me that itches when I see stuff typed in an incorrect way, as I know the next person editing the code is going to rely on the types and make assumptions based on that. I understand how doing the code properly can be a bit of a hinderance and annoyance though.

Ah yes, good point. Looks like it is being discussed microsoft/TypeScript#10421 & microsoft/TypeScript#8655.

I hear you on worrying about the next person, but I try to strike a balance for my coworkers between untyped and typed code that doesn’t become too tiresome on them :)

The beauty of TypeScript 😄

orta · 2018-02-07T13:26:10Z

src/lib/Containers/__tests__/Conversation-tests.tsx

    initial_message: "Adoro! Por favor envie-me mais informações",
+    last_message_id: "222",
    messages: {


did the relay runtime warn you about these missing fields?

Nope, the expanded type definitions did that.

alloy · 2018-02-07T13:50:51Z

Alright, going for the merge 🚀

alloy added 5 commits February 4, 2018 23:40

[package] Configure for our 1.5.0 fork.

3c5a0ad

[relay] Import artifacts.

a107e09

get types green

ea2880e

[relay] Work around a type generator bug.

f7ec283

[package] Use published DT and plugin packages.

7e1f4bf

alloy assigned orta Feb 6, 2018

alloy commented Feb 6, 2018

View reviewed changes

alloy added 2 commits February 7, 2018 10:39

[git] Collapse Relay artifacts in PRs by default.

4706d8b

[package] No need to run relay compiler now that we check them in.

8aaae50

alloy mentioned this pull request Feb 7, 2018

[WIP] Update to relay 1.4.1 #913

Closed

alloy added 2 commits February 7, 2018 11:22

[patch] Redo graphql patch without running prettier.

83edd4b

[prettier] Don’t ever reformat files in node_modules.

b2f96db

[CHANGELOG] Document Relay related changes.

0929483

orta approved these changes Feb 7, 2018

View reviewed changes

alloy mentioned this pull request Feb 7, 2018

Add schema comments to type definitions. relay-tools/relay-compiler-language-typescript#35

Open

alloy merged commit 3a41595 into master Feb 7, 2018

alloy deleted the relay-1.5.0-artsy branch February 7, 2018 13:50

damassi mentioned this pull request Feb 14, 2018

[Toolchain] Update Relay to support new TypeScript compiler option artsy/reaction#522

Merged

		@@ -1 +1,2 @@
		CHANGELOG.md merge=union
		src/__generated__/*.graphql.ts linguist-generated

		@@ -0,0 +1,2 @@
		src/__generated__/*.ts
		node_modules

[Relay] Uses latest version and our patches #982

[Relay] Uses latest version and our patches #982

Conversation

alloy commented Feb 6, 2018 • edited Loading

alloy commented Feb 6, 2018

Choose a reason for hiding this comment

ArtsyOpenSource commented Feb 7, 2018 • edited Loading

kastermester commented Feb 7, 2018

alloy commented Feb 7, 2018

orta left a 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

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

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

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

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

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

alloy commented Feb 7, 2018

alloy commented Feb 6, 2018 •

edited

Loading

ArtsyOpenSource commented Feb 7, 2018 •

edited

Loading