Sitelet https://github.com/ava/use-http/pull/251
Skip to content

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

Merged
iamthesiz merged 18 commits into
masterfrom
path-as-first-arg
Apr 23, 2020
Merged

1st argument changes, remove "path" and "url" options, bug fixes#251
iamthesiz merged 18 commits into
masterfrom
path-as-first-arg

Conversation

@iamthesiz

@iamthesiz iamthesiz commented Apr 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

JonKrone commented Apr 21, 2020

Copy link
Copy Markdown

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

iamthesiz commented Apr 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

@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

iamthesiz commented Apr 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

@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

iamthesiz commented Apr 21, 2020

Copy link
Copy Markdown
Collaborator Author

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

@iamthesiz iamthesiz mentioned this pull request Apr 23, 2020
Closed
@iamthesiz

iamthesiz commented Apr 23, 2020

Copy link
Copy Markdown
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

iamthesiz commented Apr 23, 2020

Copy link
Copy Markdown
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
@mercyaj mercyaj mentioned this pull request Apr 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@iamthesiz @JonKrone