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

Allow for parse to return a Promise #4278

Closed
wants to merge 1 commit into from
Closed

Allow for parse to return a Promise #4278

wants to merge 1 commit into from

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Nov 1, 2024

This PR enables users to return a Promise from parsing, this is mainly intended so GraphQL servers are prepared for middleware approaches that can inject async caches.

I've opted to not do validate as this could create the impression that rules can run in the background, while all rules logic is synchronous.

I've also opted to not do the parseValue and other parse equivalents as these seem less relevant for the caching bit.

😅 still have to fix a lot of types but first gauging the appeal of this.

Resolves #3421

@JoviDeCroock JoviDeCroock requested a review from a team as a code owner November 1, 2024 12:57
Copy link

netlify bot commented Nov 1, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 368d5a1
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/6724d1f20938720008afa70c
😎 Deploy Preview https://deploy-preview-4278--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 Nov 1, 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 added the PR: feature 🚀 requires increase of "minor" version number label Nov 1, 2024
@JoviDeCroock JoviDeCroock force-pushed the async-parse branch 2 times, most recently from d0349a4 to 368d5a1 Compare November 1, 2024 13:04
): DocumentNode {
const result = parse(source, options);
/* c8 ignore next 3 */
if (isPromise(result)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

As we can't really test this I've opted to ignore the lines, it's more so that consumers will be prepared to handle a Promise

@JoviDeCroock JoviDeCroock added PR: breaking change 💥 implementation requires increase of "major" version number and removed PR: feature 🚀 requires increase of "minor" version number labels Nov 1, 2024
@yaacovCR
Copy link
Contributor

yaacovCR commented Nov 5, 2024

I do not feel comfortable changing the type signature of parse() to async if it does not do async work.

I've raised #4285 as a separate feature that I think has independent value (allowing for optional simple user-provided caches) that change the type signature because the return value may be async.

@JoviDeCroock JoviDeCroock deleted the async-parse branch November 5, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: breaking change 💥 implementation requires increase of "major" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants