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

fix: #2642 root element have no __typename field #2643

Merged
merged 1 commit into from
Aug 27, 2022

Conversation

mlecoq
Copy link
Contributor

@mlecoq mlecoq commented Aug 25, 2022

Resolves #2642

@changeset-bot
Copy link

changeset-bot bot commented Aug 25, 2022

🦋 Changeset detected

Latest commit: 5c0c4c3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@urql/core Minor

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

@mlecoq mlecoq force-pushed the fix/maskTypename branch 2 times, most recently from 7079384 to 7c24b17 Compare August 25, 2022 11:49
Copy link
Collaborator

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Mind adding a test for this?

.changeset/many-worms-bow.md Outdated Show resolved Hide resolved
@mlecoq
Copy link
Contributor Author

mlecoq commented Aug 26, 2022

Mind adding a test for this?

Hi, does it look good to you after last changes ?

@kitten
Copy link
Member

kitten commented Aug 26, 2022

Hiya 👋 Sorry about that regression. That was my bad

Unfortunately, the fix here you've got now has a small problem as well.
You've got the Date instance check there, however, exchanges may add any kind of objects into the result, making the recursion now potentially dangerous and destroying instances or even recursing infinitely if instances with recursive properties are added to a result 😅

What I forgot when refactoring is that we skip adding a __typename field to the root selection set. However, what we can do about this is skip the __typename check for the first run of this function, which we could do with an additional argument for instance

@mlecoq
Copy link
Contributor Author

mlecoq commented Aug 26, 2022

Ok I'll change it

@JoviDeCroock JoviDeCroock merged commit b1ac166 into urql-graphql:main Aug 27, 2022
@urql-ci urql-ci mentioned this pull request Aug 27, 2022
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.

urql 3.0 - regression on maskTypename
3 participants