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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Jun 6, 2022
Merged

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

merged 1 commit into from
Jun 6, 2022

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented May 31, 2022 •
edited
Loading

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
Copy link

netlify bot commented May 31, 2022 •
edited
Loading

✅ 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...

QR Code

Use your smartphone camera to open QR code link.

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

@yaacovCR yaacovCR changed the title fix GraphQLWrappingType fix: GraphQLWrappingType should only wrap nullable types with non-null May 31, 2022
@yaacovCR yaacovCR added the PR: breaking change 💥 implementation requires increase of "major" version number label May 31, 2022
@yaacovCR yaacovCR requested a review from a team May 31, 2022 12:34
@yaacovCR yaacovCR mentioned this pull request May 31, 2022
Open
IvanGoncharov
IvanGoncharov previously requested changes Jun 1, 2022
View reviewed changes
Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@yaacovCR Summarising our discussion in DMs:

  1. 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.
  2. 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
Copy link
Contributor Author

yaacovCR commented Jun 1, 2022

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.

@saihaj saihaj requested a review from IvanGoncharov June 1, 2022 12:40
@yaacovCR 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 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
Copy link
Contributor Author

yaacovCR commented Jun 1, 2022

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.

@yaacovCR yaacovCR dismissed IvanGoncharov’s stale review June 1, 2022 18:46

Updated with requested changes.

@yaacovCR yaacovCR requested a review from a team June 1, 2022 18:47
@yaacovCR yaacovCR force-pushed the fix-graphql-wrapping-type branch 2 times, most recently from 86cf20e to 4d63bbf Compare June 1, 2022 18:54
@yaacovCR yaacovCR mentioned this pull request Jun 1, 2022
Merged
@yaacovCR
fix: update remaining GraphQLNonNull<GraphQLType> codepoints to
ab5a471 
`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)
@yaacovCR yaacovCR force-pushed the fix-graphql-wrapping-type branch from 4d63bbf to ab5a471 Compare June 2, 2022 19:20
@dotansimha
Copy link
Member

dotansimha commented Jun 6, 2022 •
edited
Loading

@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
Copy link
Member

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)

@IvanGoncharov 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
@IvanGoncharov
Reuse GraphQLWrappingType in 'GraphQLType' definition
60ec4ee 
Now GraphQLType is union of named types and wrapping types.
Spotted during review of graphql#3622
@IvanGoncharov IvanGoncharov mentioned this pull request Jun 6, 2022
Merged
@yaacovCR yaacovCR deleted the fix-graphql-wrapping-type branch June 10, 2022 13:16
@jasonkuhrt jasonkuhrt mentioned this pull request Oct 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Simpdaddyjosh Simpdaddyjosh Simpdaddyjosh approved these changes

@saihaj saihaj saihaj approved these changes

@n1ru4l n1ru4l n1ru4l approved these changes

@dotansimha dotansimha dotansimha approved these changes

@IvanGoncharov IvanGoncharov Awaiting requested review from IvanGoncharov

Assignees
No one assigned
Labels
PR: breaking change 💥 implementation requires increase of "major" version number
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yaacovCR @IvanGoncharov @dotansimha @n1ru4l @saihaj @Simpdaddyjosh