Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

1st argument changes, remove "path" and "url" options, bug fixes #251

Merged
merged 18 commits into from
Apr 23, 2020

Conversation

iamthesiz
Copy link
Collaborator

@iamthesiz iamthesiz commented Apr 21, 2020

Summary

  1. this removes the path and url fields in exchange for the 1st argument handling both. Auto managed useFetch doesn't append URL to provider #247
    BREAKING CHANGE 🚨
useFetch('/path')
// AND
useFetch('https://overwrite-global-url.com')
  1. Fixes issue when overwriting global options, it changes for every instance of useFetch. Deleting a global option persists for all instances #250

  2. fixes the pagination issue here loading bug with cache? #237 but might not fix the loading bug.

  3. small bug fix with responseType

Todos

  1. need more tests
    • test for useFetch('path') without the / in the path (might be fine with the current tests)
  2. codesandbox for conditional auto-managed state
  3. update youtube video for path to explain how the new way works
  4. draft a release explaining the breaking changes
  5. update all codesandboxes using path and url options. I think it's just this one and this one

@iamthesiz iamthesiz changed the title Path as 1st argument of useFetch 1st argument changes, conditional auto-managed state, overwrite global options bug Apr 21, 2020
@JonKrone
Copy link

Hey Alex!

Thanks for your work on this! I used this library for a toy project over the weekend and it was fairly seamless.

I saw in the issue thread that sparked this your consideration of a skip vs a null path. Can you share your reasoning for going with the path approach?

I have a slight preference for a skip option to avoid overriding the path parameter, though I understand that either option has some mental overhead for the library.

@iamthesiz
Copy link
Collaborator Author

iamthesiz commented Apr 21, 2020

@JonKrone I've been doing a lot of research on all the various patterns people use. It seems swr and react-query are blocking fetches with a null or falsy value. Looks like @apollo/react-hooks is using the skip option. I am open to the skip option. I wanna get some more feedback and I'll get back to you.

@iamthesiz
Copy link
Collaborator Author

iamthesiz commented Apr 21, 2020

@JonKrone After looking at it more, seems like @apollo/react-hooks has significantly more downloads and significantly more people using it. I might do the skip option as it might lead to easier understanding/adoption of the library. It's also more explicit. You are not the 1st person who has requested this feature.

@iamthesiz iamthesiz changed the title 1st argument changes, conditional auto-managed state, overwrite global options bug 1st argument changes, conditional auto-managed state, bug fixes Apr 21, 2020
@iamthesiz
Copy link
Collaborator Author

I wish there was a way to crowd source people's opinions on this.

@iamthesiz
Copy link
Collaborator Author

I found a way to crowdsource people's opinions on reddit. See the poll here on whether to use skip option or null value.

@iamthesiz iamthesiz marked this pull request as ready for review April 23, 2020 02:05
@iamthesiz iamthesiz changed the title 1st argument changes, conditional auto-managed state, bug fixes 1st argument changes, remove "path" and "url" options, bug fixes Apr 23, 2020
@iamthesiz
Copy link
Collaborator Author

Going to let more people vote before adding conditional auto-managed state. Gonna get these fixes in now though.

@iamthesiz iamthesiz merged commit 8c8937d into master Apr 23, 2020
@iamthesiz iamthesiz deleted the path-as-first-arg branch April 23, 2020 02:25
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants