[relay-runtime] Export readFragment from index and resolverDataInjector from experimental #74036
Conversation
…ct from experimental
|
@alina-zaieva Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through. This is a live comment that I will keep updated. 1 package in this PRCode ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. You can test the changes of this PR in the Playground. Status
Once every item on this list is checked, I'll ask you for permission to merge and publish the changes. InactiveThis PR has been inactive for 8 days. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 74036,
"author": "alina-zaieva",
"headCommitOid": "80daff6b697975893ecf86257822f86371b80f5b",
"mergeBaseOid": "2243b87255f228f1052ba457c4eaed37dce55047",
"lastPushDate": "2025-11-07T09:09:44.000Z",
"lastActivityDate": "2025-11-12T21:53:09.000Z",
"hasMergeConflict": false,
"isFirstContribution": true,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Popular",
"pkgInfo": [
{
"name": "relay-runtime",
"kind": "edit",
"files": [
{
"path": "types/relay-runtime/experimental.d.ts",
"kind": "definition"
},
{
"path": "types/relay-runtime/index.d.ts",
"kind": "definition"
},
{
"path": "types/relay-runtime/lib/store/live-resolvers/resolverDataInjector.d.ts",
"kind": "definition"
},
{
"path": "types/relay-runtime/package.json",
"kind": "package-meta-ok"
},
{
"path": "types/relay-runtime/relay-runtime-tests.tsx",
"kind": "test"
}
],
"owners": [
"captbaritone",
"alloy",
"maraisr",
"morrys",
"levibuzolic"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
}
],
"reviews": [
{
"type": "stale",
"reviewer": "alloy",
"date": "2025-11-12T21:41:12.000Z",
"abbrOid": "aac2b8b"
}
],
"mainBotCommentID": 3501411554,
"ciResult": "pass"
} |
|
🔔 @captbaritone @alloy @maraisr @morrys @levibuzolic — please review this PR in the next few days. Be sure to explicitly select |
| // The type returned by fields annotated with `@catch` | ||
| export type Result<T, Error> = OkayResult<T> | ErrorResult<Error>; | ||
|
|
||
| export function isValueResult<T = unknown>(input: Result<T, Error>): input is OkayResult<T>; |
There was a problem hiding this comment.
The type definition here and for isErrorResult is a bit strange in the upstream since Error generic type argument is always Error type. Not sure if that is intentional or an accident, but I've replicated the same here.
There was a problem hiding this comment.
It shouldn't matter for OkayResult, as it doesn't contain any errors, but for ErrorResult it seems like ErrorResult would lose precision and always use the Error supertype. This might be a Flow difference, which you can try on their playground.
For our needs I think we should declare the error type and perhaps even rename it (to E) to avoid confusion. So this test IMO should pass:
class CustomError < Error {}
const input: ErrorResult<CustomError> = { ok: false, errors: [new CustomError()] };
if (isErrorResult(input)) {
const e: CustomError = input.errors[0]!;
}There was a problem hiding this comment.
Confirmed in Flow playground that the behavior there matches what we have here in TypeScript - both isValueResult and isErrorResult can only accept Result values where the error is Error instance. To me this looks like a bug in Relay, but since the runtime behavior would be the same, yes, I can fix the TS typings at least 🙂
There was a problem hiding this comment.
@alloy done, isErrorResult now mirrors isValueResult and the type argument is now E instead of Error to make it clear that this is a type argument.
types/relay-runtime/lib/store/live-resolvers/resolverDataInjector.d.ts
Outdated
Show resolved
Hide resolved
| // The type returned by fields annotated with `@catch` | ||
| export type Result<T, Error> = OkayResult<T> | ErrorResult<Error>; | ||
|
|
||
| export function isValueResult<T = unknown>(input: Result<T, Error>): input is OkayResult<T>; |
There was a problem hiding this comment.
It shouldn't matter for OkayResult, as it doesn't contain any errors, but for ErrorResult it seems like ErrorResult would lose precision and always use the Error supertype. This might be a Flow difference, which you can try on their playground.
For our needs I think we should declare the error type and perhaps even rename it (to E) to avoid confusion. So this test IMO should pass:
class CustomError < Error {}
const input: ErrorResult<CustomError> = { ok: false, errors: [new CustomError()] };
if (isErrorResult(input)) {
const e: CustomError = input.errors[0]!;
}Co-authored-by: Eloy Durán <eloy.de.enige@gmail.com>
|
@alloy Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
Please fill in this template.
pnpm test <package to test>.If changing an existing definition:
readFragmentexportresolverDataInjectorIdOfandisValueResult/isErrorResultpackage.json.