simplify missingFieldHandlers

[tsnobip]

No description provided.

[zth]

I appreciate the intention here, I really do, but I'm a bit surprised that it fundamentally breaks the missing field handlers. It makes me worried that:

1. We apparently don't have tests for it
2. There are even comments above each maker fn explaining exactly why it's structured like it is, but they are neither changed (if this is a correct change) nor taken into account in the edits

Here's the Relay docs for missing field handlers: https://relay.dev/docs/next/guided-tour/reusing-cached-data/filling-in-missing-data/

[zth]

I don't think this is going to work. Unless something has changed, returning Nullable is required here, or you won't be able to distinguish between null (fetched but no data) and undefined (data not fetched). Same for the plural linked field IIRC.

[zth]

argsId is no longer turned into an option here...?

[tsnobip]

@zth my bad I first thought that ?type in flow was for optional, but indeed it's the equivalent of nullable and I forgot to fix it everywhere. I'll add tests and fix the comments too!

[zth]

@zth my bad I first thought that ?type in flow was for optional, but indeed it's the equivalent of nullable and I forgot to fix it everywhere. I'll add tests and fix the comments too!

Awesome, thank you! This is a peculiar part of the API. I guess if you wanted to you could experiment with a nicer API, like:

@unboxed
type missingFieldHandlerResult<'data> = | @as(undefined) DataNotFetched | @as(null) DataMissing | Data('data)

[tsnobip]

@zth my bad I first thought that ?type in flow was for optional, but indeed it's the equivalent of nullable and I forgot to fix it everywhere. I'll add tests and fix the comments too!

Awesome, thank you! This is a peculiar part of the API. I guess if you wanted to you could experiment with a nicer API, like:

@unboxed
type missingFieldHandlerResult<'data> = | @as(undefined) DataNotFetched | @as(null) DataMissing | Data('data)

yeah that'd be better, the whole null vs undefined API is very confusing! But looking at the actual usage of missingFieldHandlers, it looks like undefined is only tested against with scalars, otherwise they just check with != null, which means undefined and null are treated the same, so option would work there.

For scalars, I guess undefined is used for data missing and null when the data exists but is actually None or whatever you wanna call it, so maybe something like that:

@unboxed
type missingFieldHandlerResult<'data> = | @as(undefined) DataMissing | @as(null) DataIsNull | Data('data)

cf https://github.com/facebook/relay/blob/7edd02988e582570d1061ee5d5b3f9a62bafdfd7/packages/relay-runtime/mutations/createUpdatableProxy.js#L374

What do you think @zth ?

[zth]

@zth my bad I first thought that ?type in flow was for optional, but indeed it's the equivalent of nullable and I forgot to fix it everywhere. I'll add tests and fix the comments too!

Awesome, thank you! This is a peculiar part of the API. I guess if you wanted to you could experiment with a nicer API, like:

@unboxed
type missingFieldHandlerResult<'data> = | @as(undefined) DataNotFetched | @as(null) DataMissing | Data('data)

yeah that'd be better, the whole null vs undefined API is very confusing! But looking at the actual usage of missingFieldHandlers, it looks like undefined is only tested against with scalars, otherwise they just check with != null, which means undefined and null are treated the same, so option would work there.

For scalars, I guess undefined is used for data missing and null when the data exists but is actually None or whatever you wanna call it, so maybe something like that:

@unboxed
type missingFieldHandlerResult<'data> = | @as(undefined) DataMissing | @as(null) DataIsNull | Data('data)

cf https://github.com/facebook/relay/blob/7edd02988e582570d1061ee5d5b3f9a62bafdfd7/packages/relay-runtime/mutations/createUpdatableProxy.js#L374

What do you think @zth ?

We should go for whatever is in the docs, even if we interpret the code differently, unless it's super clear that the docs are wrong and the code is right. Here I'd like to stick with what the docs say. But maybe it'd be good if you read through the docs for this particular feature once more, and gave your view on how you interpret it? Maybe I'm interpreting it wrong.

[tsnobip]

We should go for whatever is in the docs, even if we interpret the code differently, unless it's super clear that the docs are wrong and the code is right. Here I'd like to stick with what the docs say. But maybe it'd be good if you read through the docs for this particular feature once more, and gave your view on how you interpret it? Maybe I'm interpreting it wrong.

I'm not saying the docs are wrong, the docs just don't talk at all about the return type of the handle function:

The handle function takes the field that is missing, the record that field belongs to, and any arguments that were passed to the field in the current execution of the query.

• When handling a 'scalar' field, the handle function should return a scalar value, in order to use as the value for a missing field
• When handling a 'linked' field, the handle function should return an ID, referencing another object in the store that should be use in place of the missing field. **

Plus they only give examples where handle returns undefined, none where it returns null, so I don't see any other solution than reading the code.

@zth I'm really not trying to nitpick on this, if you have any information about handle return types that I missed, don't hesitate! I just don't see what returning null for a missing id would mean at all.

[zth]

So, to sum up, your interpretation after reading both the docs and the code is:

• There's no code path that handles anything but returning undefined or a value, and there's nothing in the docs that indicate anything else?

If so we should go for that, then that's fine. But we should also clean up the comments in the code, if they indicate something differently.

Agreed?