Skip to content

feat: Allow skipping suggestions #4214

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

Merged
merged 3 commits into from
Oct 12, 2024
Merged

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Oct 4, 2024

Resolves #4190
Resolves #2247
Supersedes #3467

This is a best practice already in production however GraphQL JS does not provide an out of the box solution. In the community we have solutions like https://escape.tech/graphql-armor/docs/plugins/block-field-suggestions/ however in v17 it would be worth considering providing a built-in.

The recommendation here would be to do validate(schema, doc, rules, { hideSuggestions: process.env.NODE_ENV === 'production' })

@JoviDeCroock JoviDeCroock requested a review from a team as a code owner October 4, 2024 10:33
Copy link

netlify bot commented Oct 4, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 00a01cf
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/6707d7fdc530950008c1211f
😎 Deploy Preview https://deploy-preview-4214--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 configuration.

Copy link

github-actions bot commented Oct 4, 2024

Hi @JoviDeCroock, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@JoviDeCroock JoviDeCroock force-pushed the allow-skipping-suggestions branch 2 times, most recently from 08a6f22 to 13a2d6d Compare October 4, 2024 10:37
@yaacovCR
Copy link
Contributor

yaacovCR commented Oct 6, 2024

It looks like calls to didYouMean() also show up within execute(...), namely coerceInputValue() via GraphQLEnumType.parseValue() and unknown input fields on a GraphQLInputObjectType. So we may need a fourth argument to coerceInputValue() that is then passed along as a second argument to GraphQLEnumType.parseValue()? That would cause the signature of parseValue() to be different for scalars than enums => which could be avoided by passing the new argument also to scalars. The specified scalars don't give suggestions, but perhaps some custom scalars do (???) and maybe this argument could be useful. Otherwise, we could just diverge between enums and custom scalars.

@JoviDeCroock JoviDeCroock force-pushed the allow-skipping-suggestions branch from 13a2d6d to 70e4f4d Compare October 7, 2024 07:04
@JoviDeCroock
Copy link
Member Author

@yaacovCR added it for enums/scalars, just need to add tests now

@JoviDeCroock JoviDeCroock force-pushed the allow-skipping-suggestions branch 5 times, most recently from 4e16ae3 to d96813e Compare October 7, 2024 11:16
@JoviDeCroock JoviDeCroock added the PR: feature 🚀 requires increase of "minor" version number label Oct 7, 2024
Copy link
Contributor

@yaacovCR yaacovCR left a comment

Choose a reason for hiding this comment

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

I think this has been a much-requested feature! Amazing! Left some comments in terms of some suggestions, basically I am hopeful that we can make this non-breaking in terms of function signatures, even though of course it's going in v17, so we could break things if we need to.

@JoviDeCroock JoviDeCroock force-pushed the allow-skipping-suggestions branch 3 times, most recently from ac076da to 3d46cac Compare October 7, 2024 14:16
@JoviDeCroock JoviDeCroock requested a review from yaacovCR October 7, 2024 14:19
@yaacovCR
Copy link
Contributor

yaacovCR commented Oct 7, 2024

The recommendation here would be to do validate(schema, doc, rules, { maskSuggestions: process.env.NODE_ENV === 'production' })

Or validate(schema, doc, rules, { maskSuggestions: process.env.NODE_ENV !== 'development' })

@yaacovCR
Copy link
Contributor

yaacovCR commented Oct 7, 2024

I added some thoughts in #4219

@JoviDeCroock JoviDeCroock force-pushed the allow-skipping-suggestions branch 2 times, most recently from 44b356c to ff2087d Compare October 8, 2024 07:12
@JoviDeCroock JoviDeCroock force-pushed the allow-skipping-suggestions branch 2 times, most recently from 7d9166f to d6af6ea Compare October 8, 2024 07:31
@JoviDeCroock JoviDeCroock force-pushed the allow-skipping-suggestions branch from d6af6ea to 10475bb Compare October 8, 2024 07:32
Copy link
Contributor

@yaacovCR yaacovCR left a comment

Choose a reason for hiding this comment

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

Just a few questions

@JoviDeCroock JoviDeCroock force-pushed the allow-skipping-suggestions branch from 10475bb to bc0de11 Compare October 8, 2024 08:21
@JoviDeCroock JoviDeCroock force-pushed the allow-skipping-suggestions branch 2 times, most recently from 9948855 to 00a01cf Compare October 10, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants