-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
fix(gatsby): context.nodeModel.runQuery should return [] instead of null #25885
Conversation
@vladar could you provide more context on these tests that seem to affect the change in this PR. Thanks! gatsby/packages/gatsby/src/schema/__tests__/run-query.js Lines 747 to 749 in 09b3688
gatsby/packages/gatsby/src/schema/__tests__/run-query.js Lines 942 to 944 in 09b3688
|
Thanks for the PR! I think you should modify those tests to expect But as I mentioned in the issue this is likely a breaking change, so we'll keep this PR open until the next major. |
Co-authored-by: Vladimir Razuvaev <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like @vladar said, this is a small but breaking change that we can't apply on a patch or even a minor version bump.
I would 💜 this PR when we're ready to bump the major because the distinction is annoying to me (I've messed with this code a lot) and I think it's only there because the original filtering library (sift) did it this way.
But an API is an API and even though the change looks benign, you can bet there's code out there that relies on this behavior.
@@ -417,10 +417,7 @@ function convertAndApplyFastFilters( | |||
stats.totalSiftHits++ | |||
} | |||
|
|||
if (firstOnly) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc there were one or two other places where this happens (returning null
vs returning an empty array) but maybe I already unified those into this check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you want to update here @pvdz?
I'm going to put this PR in draft mode as we won't be merging it any time soon. Rest assured that I really want this PR merged once we can! |
should then the docs be updated to reflect the current state (with a deprecation warning that it would change in 3.0)? |
It's been like this for so long that I would kind of suggest to leave it as is for now. But if you're set on updating them then you could add a note that we plan to eliminate this inconsistency in a future major bump. |
# Conflicts: # packages/gatsby/src/schema/__tests__/run-query.js
Thanks for still working on getting this in @vladar, let me know if I can be of help in any way! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and let's merge it, finally! 🎉
As of now, context.nodeModel.runQuery returns
Promise<null>
, but the documentation specifies typePromise<Node[]>
to be returned.This PR resolves #25857 by updating the default return statement.