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

Call hook conditionally #269

Closed
victorivens05 opened this issue Jul 1, 2020 · 12 comments · Fixed by #272
Closed

Call hook conditionally #269

victorivens05 opened this issue Jul 1, 2020 · 12 comments · Fixed by #272
Labels
enhancement New feature or request

Comments

@victorivens05
Copy link

At some point, we need to make a request only under certain conditions, when we have a parameter for instance.
Look at this example:

  const userId = useUserIdAsync()
  const [{data}] = useAxios('/endpoint/me/${userId}')

If at first, userId is returned as null, a request to /endpoint/me/null will be made, and then another one when it receives a value. I think that's the problem the closed issue #12 tried to solve, in a wrong way, since we can't use hooks conditionally.
We can, however, tell a custom hook when we want it to fire. Example:

  const userId = useUserIdAsync()
  const [{data}] = useAxios('/endpoint/me/${userId}', { when: !!userId })

That way, the request will be made (and remade) only when the condition is met.
I'd be glad to make a PR with this functionality.

@simoneb
Copy link
Owner

simoneb commented Jul 1, 2020

This functionality already exists and it's linked in the issue you mentioned.

@simoneb
Copy link
Owner

simoneb commented Jul 1, 2020

Reopening this because upon further consideration, I realized that the current manual option alone is not enough to achieve this behavior, because options change never trigger a new request.

I'm starting to evaluate how to retrigger requests when options change, which would allow to achieve this.

Work is ongoing in #272 and a sample using the prerelease version of this change is here.

I would appreciate your feedback on this.

@simoneb simoneb reopened this Jul 1, 2020
@simoneb simoneb added the enhancement New feature or request label Jul 1, 2020
@victorivens05
Copy link
Author

I need some clarification. The expected behavior is to for the hook to request only when a certain criteria is met. In the example shown above:

  const userId = useUserIdAsync()
  const [{data}] = useAxios('/endpoint/me/${userId}')

Where two requests will be made, one with userId === null and one with userId === 1. The expected behavior can be achieved with a little more code:

  const userId = useUserIdAsync()
  const [{data}, refetch] = useAxios('/endpoint/me/${userId}', { manual: true })
  useEffect(() => {
    if (userId) refetch()
  }, [userId])

It would be easier if I could pass the condition, as boolean, for when the requests need to be made or not.
If I understood, and that's what I need clarification, #269 means to achieve this behavior this way?

  const userId = useUserIdAsync()
  const [{data}] = useAxios('/endpoint/me/${userId}', { manual: !userId })

Which will cause the hook to be manual only when the condition is not met, therefore not making a request, hence needing to request when options change?


In my opinion, there is no need to refetch when options are changed, even if another option is added (when or condition, for instance), because if they change, the url, data or params will also change.

That would make even #233 possible by having the data from the first request as a condition to make the second request. The loading would be a conjunction of both first and second loadings returned by their hooks. example:

   const {data: value, loading: loading1} = await axios.get('/api/1');
   const {data: value2, loading: loading2} = await axios.get(`/api/${value}`, {when: !!value});
   console.log({ isLoading: loading1 || loading2 })

I'm not sure, but most of the changes would be concentrated in this place?

if (!options.manual) {

Adding this option, so the request will only be made when the options.condition === true and options.manual === false

@simoneb
Copy link
Owner

simoneb commented Jul 2, 2020

I would like to avoid adding another option when the existing manual would work and doing so (possibly refetching when options change) we're also achieving a behavior that may be useful in some cases and is more generic than what you're asking.

So my question is, I believe you can achieve what you need using manual and this new behavior I pre-released. Is that the case or am I missing something?

@victorivens05
Copy link
Author

To be honest, this behavior is already achievable today, by using the manual option as shown above.
The question is, I haven't thought about it until I saw your PR (which makes me think others didn't thought either), and it still feels like a "hackish" way of solving it, not to mention it's a no brainer: the code I wrote was hard to understand and isn't self explanatory.

I recognize your unwillingness to add another param to options, but I personally think it would be easier for anyone starting to use the library to have this explicit option. Even POSTs could benefit from it, let's say you want to make a post as soon as some state is achieved. With this option this could be done completely reactively, instead of imperatively.

Answering your question, Yes, the behavior is achievable but I suggest to gather more feedback on this new option.

@simoneb
Copy link
Owner

simoneb commented Jul 2, 2020

To be honest, this behavior is already achievable today, by using the manual option as shown above.

No it isn't because requests are not re-executed when options change. It may work in your case with a new option because the userId parameter is part of the request configuration, but it's a specialization of the general case I've addressed.

The question is, I haven't thought about it until I saw your PR (which makes me think others didn't thought either), and it still feels like a "hackish" way of solving it, not to mention it's a no brainer: the code I wrote was hard to understand and isn't self explanatory.

I recognize your unwillingness to add another param to options, but I personally think it would be easier for anyone starting to use the library to have this explicit option. Even POSTs could benefit from it, let's say you want to make a post as soon as some state is achieved. With this option this could be done completely reactively, instead of imperatively.

I get your point but I don't think that anything that's not a get should ever be executed automatically.

@victorivens05
Copy link
Author

victorivens05 commented Jul 2, 2020

It may work in your case with a new option because the userId parameter is part of the request configuration

You're right, and I agree that listening to changes to options will solve a broader range of cases.

I get your point but I don't think that anything that's not a get should ever be executed automatically.

Not automatically. Reactively. Just like useEffect and how react works as a whole. But I also understand that leaving this option might lead to unexpected behavior.

@simoneb
Copy link
Owner

simoneb commented Jul 3, 2020

Not automatically. Reactively. Just like useEffect and how react works as a whole. But I also understand that leaving this option might lead to unexpected behavior.

I'd like to hear your thoughts on this but I believe it would be quite an unusual use case that you want to execute a non-GET request automatically. The hook would execute whenever the component is mounted, which is something you would usually want to do when reading data, but not when writing data.

Considering how easy it is do to so if you really need to implement this behavior, which is to use refetch in a useEffect hook explicitly, I don't think it's a scenario that we need to cover.

@victorivens05
Copy link
Author

Thanks for regarding my opinion, the world could be more like the open source community.

I looked through my coworker's and my code, and you're right. There's only one situation that would benefit from it, and it's when saving a draft, the first time the draft isn't needed to be saved, but then, everytime the state changes, a request must be made. That's already achievable without adding an option. The same apply to a log or something. All the other POST/PUT requests were imperative (directly from a user's click for instance)

I Also noted that often times we use manual and useEffect with the sole purpose of avoiding the first request. To use manual conditionally can solve this, but then the code would lose readability, so I think we won't use this technic.

All in all, I think adding this new option would avoid 1~3 lines of code without any other great gains. At this point I already think it's a bad idea.

@simoneb
Copy link
Owner

simoneb commented Jul 3, 2020

@victorivens05 I've updated the example to reproduce a use case similar to yours using the same prerelease version of axios-hooks.

If you confirm that this covers your scenario I'll make an official release with this feature.

@victorivens05
Copy link
Author

Yes, I confirm this covers my scenario.

simoneb added a commit that referenced this issue Jul 4, 2020
Fixes #269

Options are now re-evaluated when they change, which can be used to change the behavior

of the component where the hook is used based on internal component state.

For example, the `manual` option could be initially true and then set to false based on

the value of a state property, causing the request to fire when this happens.

Similarly, the `useCache` option could be set to true (its default value) then to false

based on a condition, forcing the next request to skip the cache.

Check out [this example](https://codesandbox.io/s/axios-hooks-options-change-v23tl)
and the docs for additional information.
@simoneb
Copy link
Owner

simoneb commented Jul 4, 2020

This is now available in version 2.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants