polish: use closure instead of CollectFieldsContext #4361
yaacovCR
wants to merge
1 commit into
graphql:next
from
yaacovCR:use-closure
github-actions111 days ago
Hi @yaacovCR, 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
yaacovCR111 days ago (edited 111 days ago)
Note: this would probably be most helpfully reviewed by ignoring whitespace differences and possibly side-by-side comparison.
yaacovCR
changed the title
yaacovCR111 days ago
Also subtracts about 50 lines of code, which is always good.
From my machine:
yaacovCR111 days ago
Also subtracts about 50 lines of code, which is always good.
Mostly whitespace/formatting though, so your mileage may vary, but it helps make the code more readable for me.
use closure instead of CollectFieldsContext
836d4a50
Login to write a write a comment.
Reviewers
No reviews
Assignees
No one assigned
Labels
PR: polish 💅
Milestone
No milestone
While working on #4360, I felt myself trying to reach for opportunity to remove the CollectFieldsContext type/object that I previously introduced; this is accomplished in this separate PR.
Benchmarking on my machine, this seems essentially no better and no worse in the performance realm. I am evaluating this in terms of readability, both in the local sense and in the overall algorithmic sense of better understanding the method and limit of how we are recursing.
Overall, I think it's a minor improvement.