fix: update remaining `GraphQLNonNull<GraphQLType>` codepoints to `GraphQLNonNull<GraphQLNullableType>` by yaacovCR · Pull Request #3622 · graphql/graphql-js

yaacovCR · 2022-05-31T12:32:59Z

This is related to #3597, in the sense that #3597 made the GraphQLNonNull<GraphQL*Type> => GraphQLNonNull<GraphQLNullable*Type> change in other code points related to the isNonNullType function, but not within GraphQLWrappingType or assertNonNullType.

This PR was prompted by the uncovering of a bug by a different PR, see: #3617 (comment)

netlify · 2022-05-31T12:33:06Z

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	`ab5a471`
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62990d875de8a10009a8213d
😎 Deploy Preview	https://deploy-preview-3622--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

IvanGoncharov

Looks good!

But as previously discussed you need to provide a description of your change, currently, it only contains links.

Also, bug fixes should include tests, otherwise, we have no way to detect regressions.

IvanGoncharov · 2022-06-01T11:29:53Z

Also, bug fixes should include tests, otherwise, we have no way to detect regressions.

@yaacovCR Summarising our discussion in DMs:

I got confused with the PR description and jump to conclusion that this is recent regression as a result of Fix typing for isNonNullType #3597, which is not true. And this is exactly the reason why we really need descriptions even on small PRs.
I assumed that we have at least some tests for isWrappingType and it was a matter of adding a line or two of code that asserts TS types, which is not the case. After checking our source I agree that enforcing tests on this one is beyond a scope of this PR.

So I fully agree that in this context, requirement for adding tests was arbitrarily on my part.
Ideally, we would have tests added for all bug fixes but in this particular case adding tests is disproportional to the amount of actual change.

yaacovCR · 2022-06-01T11:37:34Z

We all can and do make mistakes, especially when busy. An extra note explicitly stating that this was a long standing bug could not have hurt, sure.

yaacovCR · 2022-06-01T18:45:12Z

I updated the description above. For good measure, I did a quick search for additional code points to correct and udpated the title of this PR.

Updated with requested changes.

`GraphQLNonNull<GraphQLNullableType>` This is related to graphql#3597, in the sense that graphql#3597 made the `GraphQLNonNull<GraphQL*Type>` => `GraphQLNonNull<GraphQLNullable*Type>` change in other code points related to the `isNonNullType` function, but not within `GraphQLWrappingType` or assertNonNullType. This PR was prompted by the uncovering of a bug by a different PR, see: graphql#3617 (comment)

dotansimha · 2022-06-06T02:27:06Z

@IvanGoncharov Any chance you can review this PR again?
At the moment, we don't have TypeScript types testing setup, and from my experience with codegen and other libraries, it is not always easy to add TypeScript types testing.
Do you think we can merge this one and maybe open an issue to improve TS types testing later? (also because this PR is fairly small and simple, and it's really addressing a real TS types bug)

IvanGoncharov · 2022-06-06T12:25:14Z

At the moment, we don't have TypeScript types testing setup, and from my experience with codegen and other libraries, it is not always easy to add TypeScript types testing.
Do you think we can merge this one and maybe open an issue to improve TS types testing later? (also because this PR is fairly small and simple, and it's really addressing a real TS types bug)

Agree, answered here #3622 (comment)

Now GraphQLType is union of named types and wrapping types. Spotted during review of graphql#3622

…aphql#3622) `GraphQLNonNull<GraphQLNullableType>` This is related to graphql#3597, in the sense that graphql#3597 made the `GraphQLNonNull<GraphQL*Type>` => `GraphQLNonNull<GraphQLNullable*Type>` change in other code points related to the `isNonNullType` function, but not within `GraphQLWrappingType` or assertNonNullType. This PR was prompted by the uncovering of a bug by a different PR, see: graphql#3617 (comment)

) `GraphQLNonNull<GraphQLNullableType>` This is related to #3597, in the sense that #3597 made the `GraphQLNonNull<GraphQL*Type>` => `GraphQLNonNull<GraphQLNullable*Type>` change in other code points related to the `isNonNullType` function, but not within `GraphQLWrappingType` or assertNonNullType. This PR was prompted by the uncovering of a bug by a different PR, see: #3617 (comment)

…aphql#3622) `GraphQLNonNull<GraphQLNullableType>` This is related to graphql#3597, in the sense that graphql#3597 made the `GraphQLNonNull<GraphQL*Type>` => `GraphQLNonNull<GraphQLNullable*Type>` change in other code points related to the `isNonNullType` function, but not within `GraphQLWrappingType` or assertNonNullType. This PR was prompted by the uncovering of a bug by a different PR, see: graphql#3617 (comment)

) `GraphQLNonNull<GraphQLNullableType>` This is related to #3597, in the sense that #3597 made the `GraphQLNonNull<GraphQL*Type>` => `GraphQLNonNull<GraphQLNullable*Type>` change in other code points related to the `isNonNullType` function, but not within `GraphQLWrappingType` or assertNonNullType. This PR was prompted by the uncovering of a bug by a different PR, see: #3617 (comment)

yaacovCR changed the title ~~fix GraphQLWrappingType~~ fix: GraphQLWrappingType should only wrap nullable types with non-null May 31, 2022

yaacovCR added the PR: breaking change 💥 implementation requires increase of "major" version number label May 31, 2022

yaacovCR requested a review from a team May 31, 2022 12:34

yaacovCR mentioned this pull request May 31, 2022

feat: replace instanceOf with unique Symbol checks #3617

Closed

n1ru4l approved these changes May 31, 2022

View reviewed changes

saihaj approved these changes Jun 1, 2022

View reviewed changes

IvanGoncharov previously requested changes Jun 1, 2022

View reviewed changes

saihaj requested a review from IvanGoncharov June 1, 2022 12:40

yaacovCR changed the title ~~fix: GraphQLWrappingType should only wrap nullable types with non-null~~ fix: change remaining GraphQLNonNull<GraphQLType> codepoints to GraphQLNonNull<GraphQLNullableType> Jun 1, 2022

yaacovCR changed the title ~~fix: change remaining GraphQLNonNull<GraphQLType> codepoints to GraphQLNonNull<GraphQLNullableType>~~ fix: update remaining GraphQLNonNull<GraphQLType> codepoints to GraphQLNonNull<GraphQLNullableType> Jun 1, 2022

yaacovCR requested a review from a team June 1, 2022 18:47

yaacovCR force-pushed the fix-graphql-wrapping-type branch 2 times, most recently from 86cf20e to 4d63bbf Compare June 1, 2022 18:54

yaacovCR mentioned this pull request Jun 1, 2022

diff-npm-package: fix reports dir creation #3624

Merged

yaacovCR force-pushed the fix-graphql-wrapping-type branch from 4d63bbf to ab5a471 Compare June 2, 2022 19:20

dotansimha approved these changes Jun 6, 2022

View reviewed changes

IvanGoncharov merged commit 1de3bd3 into graphql:main Jun 6, 2022

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jun 6, 2022

Reuse GraphQLWrappingType in 'GraphQLType' definition

60ec4ee

Now GraphQLType is union of named types and wrapping types. Spotted during review of graphql#3622

IvanGoncharov mentioned this pull request Jun 6, 2022

Reuse GraphQLWrappingType in 'GraphQLType' definition #3627

Merged

yaacovCR deleted the fix-graphql-wrapping-type branch June 10, 2022 13:16

Simpdaddyjosh approved these changes Apr 11, 2023

View reviewed changes

jasonkuhrt mentioned this pull request Oct 29, 2024

refactor: treeshakable kind enum #4270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update remaining `GraphQLNonNull<GraphQLType>` codepoints to `GraphQLNonNull<GraphQLNullableType>` #3622

fix: update remaining `GraphQLNonNull<GraphQLType>` codepoints to `GraphQLNonNull<GraphQLNullableType>` #3622
IvanGoncharov merged 1 commit intographql:mainfrom
yaacovCR:fix-graphql-wrapping-type

yaacovCR commented May 31, 2022 •

edited

Loading

Uh oh!

netlify Bot commented May 31, 2022 •

edited

Loading

Uh oh!

IvanGoncharov left a comment

Uh oh!

IvanGoncharov commented Jun 1, 2022

Uh oh!

yaacovCR commented Jun 1, 2022

Uh oh!

yaacovCR commented Jun 1, 2022

Uh oh!

dotansimha commented Jun 6, 2022 •

edited

Loading

Uh oh!

IvanGoncharov commented Jun 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

yaacovCR commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify Bot commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Uh oh!

IvanGoncharov left a comment

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov commented Jun 1, 2022

Uh oh!

yaacovCR commented Jun 1, 2022

Uh oh!

yaacovCR commented Jun 1, 2022

Uh oh!

dotansimha commented Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IvanGoncharov commented Jun 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yaacovCR commented May 31, 2022 •

edited

Loading

netlify Bot commented May 31, 2022 •

edited

Loading

dotansimha commented Jun 6, 2022 •

edited

Loading