Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Add primitives for variables and responses #606

Closed

Conversation

@andyrichardson
Copy link
Contributor

andyrichardson commented Mar 12, 2020

@changeset-bot

This comment has been minimized.

Copy link

changeset-bot bot commented Mar 12, 2020

💥 No Changeset

Latest commit: 5d9705f

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@andyrichardson andyrichardson requested review from JoviDeCroock and sofiapoh Mar 12, 2020
Co-authored-by: Sofia Pohjalainen <sofia.pohjalainen@formidable.com>
@andyrichardson andyrichardson force-pushed the 604-update-types-for-data-and-variables branch from 77393ae to 31ec3f2 Mar 12, 2020
Copy link
Member

kitten left a comment

Overall I think we can land this in a major. We'll just have to document that users may be able to pass any for the Data generic explicitly to get the current behaviour back 👍

@@ -60,7 +68,7 @@ export interface Operation extends GraphQLRequest {
}

/** Resulting data from an [operation]{@link Operation}. */
export interface OperationResult<Data = any> {
export interface OperationResult<Data extends PrimitiveMap = PrimitiveMap> {

This comment has been minimized.

Copy link
@kitten

kitten Mar 12, 2020

Member
Suggested change
export interface OperationResult<Data extends PrimitiveMap = PrimitiveMap> {
export interface OperationResult<Data = PrimitiveMap | null> {

Let's make sure that we also take null into account. I'd either add it here or below to data?: Data | null, since it can always happen when the entire GraphQL endpoint errors out.

Also, I don't think we can add extends PrimitiveMap because the result may not be serialisable after it passed through graphcache or other custom exchanges that may add special data, so let's assume the user knows what they're doing when they pass in a Data generic that doesn't extend PrimitiveMap.

Also, I realised that users can get the "legacy behaviour" back by just passing any to the Data generic, so no harm done I guess.

packages/core/src/types.ts Outdated Show resolved Hide resolved
/** A Graphql query, mutation, or subscription. */
export interface GraphQLRequest {
/** Unique identifier of the request. */
key: number;
query: DocumentNode;
variables?: object;
variables?: PrimitiveMap;

This comment has been minimized.

Copy link
@kitten

kitten Mar 12, 2020

Member

I think this change should be made to the framework bindings, e.g. useQuery etc, in particular by adding it to the generic there like we do below for OperationResult. That's because someone may be using variables to add some non-serialisable metadata, like Date, which would then be stringified by stringifyVariables. It may also be used to pass variables that are only used offline and then filtered out, so I'm a little uneasy about making more assumptions here

This comment has been minimized.

Copy link
@andyrichardson

andyrichardson Mar 18, 2020

Author Contributor

Yeah I was thinking of the toString issue - would adding something like this to the primitives help?

I'm personally in the mindset that users should be making sure that their variables should be intentionally/explicitly converting beforehand but I appreciate that enforcing that isn't a great idea.

This comment has been minimized.

Copy link
@kitten

kitten Mar 22, 2020

Member

It's unlikely 😅 We now also support passing File and Blob variables for @urql/exchange-multipart-fetch and cases like these make it hard for us to have a stricter type by default 🤔

@kitten kitten changed the title Add primitives for variables and responses Proposal: Add primitives for variables and responses Mar 12, 2020
Co-Authored-By: Phil Plückthun <phil@kitten.sh>
@andyrichardson andyrichardson deleted the 604-update-types-for-data-and-variables branch Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.