-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this! 💯 Great work Andy!
We still need to add the export though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also add the exports in a separate PR if needed 👍 There's still some things that we can adapt in here, that should go in a separate PR
The console.warn
and throw Error
can be replaced with our warning
and invariant
helpers and there's probably some other stuff that can be reused 💯
Then we do need to add codes for them |
Types look pretty vague on Invariant (i.e. what is code?) - let me know if I'm doing this wrong. Also, would be nice to do if (condition)
throw invariant(...args)
}
someOtherCode(); rather than if (condition) {
invariant(...args); // Intention to throw isn't clear to newcomers
}
someOtherCode(); |
Done 👍 |
Looked at typing the errors but I'm not sure numbers is the way to do this. String literals would make more sense: Numbers // Types
type ErrorCode = InvalidGraphQLDocumentCode | InvalidCacheCallCode;
type InvalidGraphQLDocumentCode = 1;
type InvalidCacheCallCode = 2;
// Usage
warn(msg, 1); // 1 ???
// User
if (err.code === 1) // What does this mean? Literals // Types
type ErrorCode = "INVALID_GRAPHQL_DOCUMENT" | "INVALID_CACHE_CALL";
// Usage
warn(msg, "INVALID_GRAPHQL_DOCUMENT"); // makes more sense
// User
if (err.code === "INVALID_GRAPHQL_DOCUMENT") Breaking change but removes the confusion when catching errors (most users won't be catching anyhow) |
@andyrichardson They're not any typed/relevant type information, but they're just numeric identifiers that we add to our So typically just choose the next highest number in that doc and add a new section and copy that info over 😅 It's not linted (yet) Re: Throw vs invariant; it's a pattern we're basically adopting from React and other libraries. Happy to also open a PR after we merge this that does all of this at the same time |
@kitten yeah I totally get what the numbers are for. Literals would do the same job, just a better DX. Going back on topic, what's left for this PR? The only thing I'm aware of is the number being thrown from the invariant call. Can't see anyone looking to wrap the exchange in a try/catch so I wouldn't put too much thought into it. |
invariant( | ||
'PopulateExchange', | ||
'Invalid TypeInfo state. "getTypeName" does not expect to receive an abstract type.', | ||
1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still an error code that isn't in help.md
but I'll fix that up in the branch I've mentioned, which I'll have to rebase anyway :)
For discussion, see urql-graphql/urql#471