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

[Docs] Remove optional initialQueryRef arg from use-query-loader.md example #4054

Closed
wants to merge 3 commits into from

Conversation

damassi
Copy link
Contributor

@damassi damassi commented Aug 29, 2022

As per this stack overflow question, the docs are confusing on first glance. It looks like the user needs to provide an initialQueryRef argument and the docs reference a router, but thats quite a bit out of context, and the argument isn't needed.

I imagine quite a few devs have looked at this example, couldn't figure out how to obtain the initialQueryRef and have given up.

This removes the reference from the code example for clarity sake and a follow-up PR should add a fully working example where the initial query ref is passed.

@rbalicki2
Copy link
Contributor

Good call, thanks for this change!

For additional context, the behavior of this parameter is somewhat broken: if useQueryLoader is passed an initialQueryRef and the hook commits, it will dispose of the initial query ref on unmount. If it is passed one and doesn't commit, it won't. (The behavior is also probably sketchy if you call loadQuery or dispose before commit.)

But: this hook should dispose of the initial query ref, though, because it doesn't own that query ref. So, e.g. a parent useQueryLoader's query ref, which is being used as an initial query ref here, will now be disposed and in an invalid state.

A better pattern (in all cases) is to do queryRef ?? someInitialQueryRef wherever we use the query ref with a default. This also has the benefit of being more accurately typed.

@facebook-github-bot
Copy link
Contributor

@rbalicki2 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants