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

Provide possibility to mask the __typename in client results #533

Merged
merged 20 commits into from
Feb 13, 2020

Conversation

JoviDeCroock
Copy link
Collaborator

@JoviDeCroock JoviDeCroock commented Feb 11, 2020

Summary

It solves problems where you don't want to manually strip in variables/useQuery result before sending it off somewhere

Set of changes

  • add export to urql/core (maskTypename)
  • adds the option to client to enable this on all results

@changeset-bot
Copy link

changeset-bot bot commented Feb 11, 2020

🦋 Changeset is good to go

Latest commit: f14497b

We got this.

Not sure what this means? Click here to learn what changesets are.

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

@JoviDeCroock JoviDeCroock requested a review from kitten February 11, 2020 12:05
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 11, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e9112f1:

Sandbox Source
still-dust-ccucq Configuration

Copy link
Member

@kitten kitten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it conceptually make more sense to be able to use stripTypename on variables?

@JoviDeCroock
Copy link
Collaborator Author

Doesn't it conceptually make more sense to be able to use stripTypename on variables?

So to me it makes sense being able to use it on the response for your query since we implicitly add __typename. This allows us to reverse that (optionally). variables with __typename seems like a bad practice for a request so I enforced it there? What would you change?

@kitten
Copy link
Member

kitten commented Feb 11, 2020

@JoviDeCroock That's true. What I'm unsure of is that we basically have an asymmetry already:

  • We add __typename but that cannot be disabled or reversed on query documents
  • We may remove __typename but that's on a hook level in this PR
  • Removing __typename is often relevant when passing results as inputs but we'd be applying it to results instead of to variables

So I'm not sure whether the place where we're applying this makes sense?

@JoviDeCroock
Copy link
Collaborator Author

JoviDeCroock commented Feb 11, 2020

The main point you are hinting at is to keep it exclusively to useRequest right? I just think it's very ergonomic to provide this option (default false) to useQuery in case of for instance initialValues of a form, when using object schema validation additional properties often result in errors.

.changeset/modern-queens-run.md Outdated Show resolved Hide resolved
.changeset/modern-queens-run.md Outdated Show resolved Hide resolved
@marcusdarmstrong
Copy link

The implication of these changes would be that it's impossible to useQuery to fetch __typename as a part of the original query? That seems harmful.

@JoviDeCroock
Copy link
Collaborator Author

The implication of these changes would be that it's impossible to useQuery to fetch __typename as a part of the original query? That seems harmful.

No, we've always implicitly added __typename and result.data.field.__typename will still be defined it just won't be enumerable anymore

@marcusdarmstrong
Copy link

The implication of these changes would be that it's impossible to useQuery to fetch __typename as a part of the original query? That seems harmful.

No, we've always implicitly added __typename and result.data.field.__typename will still be defined it just won't be enumerable anymore

  1. __typename is only automatically added via cache exchanges presently, not urql itself—our usage doesn't involve that automatic addition, as we don't use the default cache or graphcache—and that's actually important for us, as we'd like to move toward not actually having query ASTs on the client/in our bundles.
  2. Enumerability is the expectation for fields I request in my query. That makes this a breaking change.

@kitten
Copy link
Member

kitten commented Feb 12, 2020

What this PR doesn't include yet is that this should and will only be enabled when an option on the client is enabled, e.g. new Client({ maskTypenames: true })

JoviDeCroock and others added 3 commits February 12, 2020 18:09
Co-Authored-By: Phil Plückthun <phil@kitten.sh>
Co-Authored-By: Phil Plückthun <phil@kitten.sh>
@JoviDeCroock JoviDeCroock changed the title Strip typename in useQuery (optional) and in useRequest variables Provide possibility to mask the __typename in client results Feb 13, 2020
@JoviDeCroock JoviDeCroock requested a review from kitten February 13, 2020 10:56
.changeset/modern-queens-run.md Outdated Show resolved Hide resolved
Co-Authored-By: Phil Plückthun <phil@kitten.sh>
@JoviDeCroock JoviDeCroock merged commit c7d6b45 into master Feb 13, 2020
@JoviDeCroock JoviDeCroock deleted the feat/strip-typename branch February 13, 2020 13:10
@kitten
Copy link
Member

kitten commented Jun 27, 2024

maskTypename has been removed.
For a full write-up on why and more see: #3299 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants