Fix opaque types

[ds300]

This PR fixes issue #138.

Quick recap:

In TS 3.6 our 'opaque types' became unsafe as a result of refinement in the logic around how certain types intersect.

This PR works around that by combining types in a different way. Please refer to #138 for a discussion of the approach and rationale.

Note that by using string literals instead of unique symbols we gain improved DX in the form of more readable type errors. By switching the " $refType" property to use string literals we also simplify the implementation by avoiding the need to manage type imports.

[kastermester]

I like where this is headed. I’m on my phone right now so this is not a full review - I just had a quick glance at it.

I would think this will also allow our users to use Relay without using the single artifact directory - is that correct?

Also I think the “$ref” suffix can be dropped in the reftype property (/ I think it should be dropped to mimic the stuff going on with fragment refs. Also I would think it reads nicer?)

Looking good!

[ds300]

I would think this will also allow our users to use Relay without using the single artifact directory - is that correct?

I'm not familiar with the history of this restriction, but if it's down to the complexity of managing import paths then yeah as long as names are still globally unique.

Also I think the “$ref” suffix can be dropped in the reftype property (/ I think it should be dropped to mimic the stuff going on with fragment refs. Also I would think it reads nicer?)

I agree it reads nicer without the $ref. That's how I did it originally but I added the $ref back for a couple of reasons

• it makes it even more obvious that this string value is not something the user should be writing.
• it avoids the implication that " $refType" property is directly related to " $fragmentRefs"

Both of these are super minor so I'm happy remove it again if you like?

[kastermester]

I would think this will also allow our users to use Relay without using the single artifact directory - is that correct?

I'm not familiar with the history of this restriction, but if it's down to the complexity of managing import paths then yeah as long as names are still globally unique.

As far as I recall this is exactly the issue going on here.

Also I think the “$ref” suffix can be dropped in the reftype property (/ I think it should be dropped to mimic the stuff going on with fragment refs. Also I would think it reads nicer?)

I agree it reads nicer without the $ref. That's how I did it originally but I added the $ref back for a couple of reasons

• it makes it even more obvious that this string value is not something the user should be writing.

I agree that would be a nice quality to have - but I'm not sure this quality. Also the propery name itself hopefully somewhat conveys this message.

• it avoids the implication that " $refType" property is directly related to " $fragmentRefs"

They are directly related though. In order to pass a property down the component tree to match with regards to relay the value has to have the $refType contained within the $fragmentRefs of the data being passed down. This means the 2 values has to match.

Both of these are super minor so I'm happy remove it again if you like?

Unless I have missed something I think this is a requirement. Have you tested a build with this PR on an actual app? :)

[alloy]

Looking nice! 👌

I would think this will also allow our users to use Relay without using the single artifact directory - is that correct?

I'm not familiar with the history of this restriction, but if it's down to the complexity of managing import paths then yeah as long as names are still globally unique.

As far as I recall this is exactly the issue going on here.

Correct–and yes, relay-compiler should be enforcing unique GraphQL document naming.

This is a huge win 🚀

Have you tested a build with this PR on an actual app? :)

(@kastermester FYI @ds300 is a team member of mine, so we’ll definitely be testing it on our regular OSS apps.)

This is indeed a good question, especially as these changes should be requiring changes to the DT typings and so I’d like to see a PR on one of our OSS apps that has patch-package patches for the required @types/* changes.

[ds300]

They are directly related though. In order to pass a property down the component tree to match with regards to relay the value has to have the $refType contained within the $fragmentRefs of the data being passed down. This means the 2 values has to match

Ah thanks, I only just looked at how that works 😅

I've updated it, and also tested it against the example app in the repo (which needed some spring cleaning). It all worked but we would need to make a breaking change to @types/relay-runtime. I wonder if there's a way around that? 🤔

[alloy]

It all worked but we would need to make a breaking change to @types/relay-runtime. I wonder if there's a way around that? 🤔

Unsure, but I will say that this minor version bump of TS led to breakage in our type setup so if we can’t really work around that then I don’t think we need to worry about it too much. Put differently, I assume all users want working types and so having to do some upgrade work is going to be needed.

[ds300]

OK this should be good to go if we update the relay-runtime types in DefinitelyTyped with the patch contents in this PR. example/patches/@types+relay-runtime+6.0.6.patch

[ds300]

Worth noting that we've successfully tested these changes now both on the example app in this repo, and on Artsy's react-native repo emission.

[alloy]

We’re holding-off until we know the https://github.com/relay-tools/relay-compiler-language-typescript release process is in a good place. /cc @zephraph

[orta]

[alloy]

They ran out of spuds!

[alloy]

Ok, release process is fixed, so going for it now

[alloy]

🚀 PR was released in v9.0.0 🚀