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

Add new 'GraphQLSchema.getField' method #3605

Merged
merged 1 commit into from May 27, 2022

Conversation

IvanGoncharov
Copy link
Member

Motivation: generalize it and remove code dublication in two places and
also allow to use as public API outside of graphql-js

@netlify
Copy link

netlify bot commented May 25, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 475bf7d
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/628e803f8ad7790009d769c2
😎 Deploy Preview https://deploy-preview-3605--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.

@IvanGoncharov
Copy link
Member Author

cc @yaacovCR

@github-actions
Copy link

Hi @IvanGoncharov, 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

@IvanGoncharov

This comment has been minimized.

@github-actions
Copy link

@github-actions run-benchmark

@IvanGoncharov Please, see benchmark results here: https://github.com/graphql/graphql-js/runs/6598090837?check_suite_focus=true#step:6:1

@IvanGoncharov
Copy link
Member Author

IvanGoncharov commented May 25, 2022

@IvanGoncharov Please, see benchmark results here: graphql/graphql-js/runs/6598090837?check_suite_focus=true#step:6:1

Sadly, I need to figure out a way to run benchmark reliably on CI after #3604
But here is data from my machine where HEAD is this PR.
image

So performance slightly increases by ~1% due to using a switch instead of multiple if.

@IvanGoncharov IvanGoncharov marked this pull request as ready for review May 25, 2022 19:13
Motivation: generalize it and remove code dublication in two places and
also allow to use as public API outside of graphql-js
@IvanGoncharov IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label May 27, 2022
@IvanGoncharov IvanGoncharov merged commit 69e1554 into graphql:main May 27, 2022
@IvanGoncharov IvanGoncharov deleted the pr_branch2 branch May 27, 2022 19:19
@yaacovCR
Copy link
Contributor

Because this PR contains two changes ie change to switch and addition of unnecessary check for unions during execution, the benchmark results are not so accurate.

i have suggested adding two helpers. See #3519 (comment)

getObjectField and getCompositeField maybe?

@yaacovCR
Copy link
Contributor

Sadly, I need to figure out a way to run benchmark reliably on CI after #3604

So it seems like #3604 broke benchmarking? Can we revert at least until fixed? Can we consider a 3rd party benchmarking solution as an additional dev-dependency so that we can outsource some of the benchmarking work?

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.

None yet

2 participants