-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
Feature Requests and Syntax Ideas #13
Comments
I like the idea passing an abort controller to the library, gives the user the ability to override functionality if they want to notify user of an aborted request use a mock controller in testing, for limiting requests i would go with |
What do we think about this syntax for import React, { Fragment } from 'react'
import { Provider, useQuery, useMutation } from 'use-http'
function QueryComponent() {
const request = useQuery`
query Todos($userID string!) {
todos(userID: $userID) {
id
title
}
}
`
const getTodosForUser = id => request.query({ userID: id })
return (
<Fragment>
<button onClick={() => getTodosForUser('theUsersID')}>Get User's Todos</button>
{request.loading ? 'Loading...' : <pre>{request.data}</pre>}
</Fragment>
)
}
const App = () => (
<Provider url='https://example.com'>
<QueryComponent />
</Provider>
) The main difference here is the string template literal here. aka the const request = useQuery`my query here` instead of const request = useQuery(`my query here`) |
I guess it wouldn't bee too difficult to support both |
@dagda1 what do you think of this syntax? |
@alex-cory I'm not too up to speed with graphql. I do remember that literal syntax from a contract that used styled-components ages ago. I'd like to be taking advantage of typescript for these queries, string queries give me the heebies. I did a spike with this ages ago but not sure I can give much of an opinion here apart from that. |
Interesting. I will check it out :) Thanks for the feedback! |
@alex-cory the reason I stumbled across your library is that I need something that does retries. Would you be open to me having a stab at a PR for retries? |
@dagda1 Yes! There's actually a TODO at the bottom of the readme for this. I was thinking there could be an option that would look like const request = useFetch(url, {
retries: 3
// OR
retry: 3
}) and after the 3rd retry it will throw an error into |
@alex-cory I think you would want a delay between each retry so you could have |
Has a useReducer implementation been considered? I ask this partly because I haven't figure out how to use use-http in a scenario where the data was an array that would need to be added to later. onClick to get more items, for example. I think the existing functionality could could be replicated with a default reducer and useReducer instead of the three useState's in useFetch.ts. From the user's perspective, there would no change unless they supplied a reducer, and possibly an initial state. I have my own naive implementation of this, reusing use-http's FetchContext, some copy/paste from useFetch.ts, and dispatch instead of setData, setError, and setLoading. It seems to work. Any thoughts? Thanks for the lib. |
@violinchris I have thought about this. const Todos = () => {
const [todos, setTodos] = useState([])
const [initialTodos, loadingInitialTodos, _, getInitialTodos] = useGet(url)
const [newTodo, loadingNewTodo, _, postNewTodo] = usePost(url)
useEffect(() => {
getInitialTodos()
}, [])
// set initial todos
if (todos.length === 0 && !loadingInitialTodos) {
setTodos(initialTodos)
}
const addTodo = useCallback(async () => {
await postNewTodo({ title: 'new todo' })
setTodos([...todos, newTodo])
}, [postNewTodo, newTodo])
return <>
{todos.map(todo => <div key={todo.id}>{todo.title}</div>}
<button onClick={addTodo}>Add Todo</button>
</>
} (there might be some holes in this^ I did it really fast) |
I'm even less certain about this than I was when I posted my reducer comment. I came across a few things as I tried to get this your example working. I had to change I never got the Anyway, I mention all of this not to nitpick with your code. I could still be doing multiple things wrong. It's that the style of coding necessary seems error prone with the multiple conditions in the if statements, and not being able to easily figure out why a render is happening, yet having to rely on a specific re-render in the if statement. Specifically regarding the syntax, here is what I was thinking about.
I'm less convinced than earlier that this is worth it, but I don't see a downside. It would function as it currently does if someone chose to not use the dispatch/reducer. And its the ultimate opt-out for someone that decides they like this library, but not for everything. Thanks again. Its useful to me regardless, and I'll keep watching the repo. |
@violinchris did you notice that I changed my example above to include an |
I like the concept of initial data, in my half arsed and half baked first attempt at export const useFetchData = <TData, TReturnData = TData>(
fetchUrl: string,
defaultValue: TData,
transformFunc: (o: TData) => TReturnData = identity,
): UseFetchData<TReturnData> => { |
@alex-cory i did not notice the await. thats a nice trick, and it ensures setTodos([...todos, newTodo]) happens only once for each addTodo. I noticed the stale newTodo. would this be necessary
and later after the await, using the updater form of the set function
More importantly, to prevent errors and re-render problems, I did have to alter the if statement.
I say more importantly because my whole reason for asking for an options.reducer and to return the dispatch was so that I could interact with the data. Instead of that, could the
Doesn't that remove the need for the @dagda1 It's probably not a top priority, but +1 for "transform function". i had the same idea, but I didn't have the courage to request. |
Interesting. I kind of like that idea. I need to think about that a little bit. @violinchris |
I think you are right with the |
🤔What about something like this? const TestApp = () => {
const [todos, setTodos] = useState([])
// const [data, loading, error, request, setData] = useFetch()
const request = useFetch('http:example.com', {
// initial data, probably just an empty default as this will get overwritten
// each time `request.method()` is called
data: []
})
useEffect(() => {
initializeTodos()
}, [])
async function initializeTodos() {
// const { data, endLoading } = await request.get('/todos')
const [ initialTodos, stopLoading ] = await request.get('/todos')
setTodos(initialTodos)
stopLoading()
}
async function addTodo() {
const [ todo, stopLoading ] = await request.post('/todos', {
title: 'No way...'
})
setTodos(oldTodos => [...oldTodos, newTodo])
stopLoading()
}
} I like this better but it'll require using js const TestApp2 = () => {
const [todos, setTodos] = useState([])
// const [data, loading, error, request, setData] = useFetch()
const request = useFetch('http:example.com', {
// initial data, probably just an empty default as this will get overwritten
// each time `request.method()` is called
data: []
})
useEffect(() => {
initializeTodos()
}, [])
async function initializeTodos() {
const initialTodos = await request.get('/todos')
setTodos(initialTodos)
request.loading = false // -> request.setLoading(false)
}
async function addTodo() {
const newTodo = await request.post('/todos', {
title: 'No way...'
})
setTodos(oldTodos => [...oldTodos, newTodo])
request.loading = false
}
return (
<>
<button onClick={addTodo}>Add Todo</button>
{request.error && 'Error!'}
{request.loading && 'Loading...'}
{(todos.data || []).length > 0 && todos.data.map(todo => (
<div key={todo.id}>{todo.title}</div>
)}
</>
)
} |
At some point in the past, I think data wasn't being returned from what is now doFetch in useFetch.ts, but now it is. That was my assumption anyway, esp. when looking at your first todo example that added the newTodo from the Anyway, with these examples, this all seems more conventional in terms of how i think about async stuff, but also it seems more likely that a certain boilerplate will be the usual procedure.
This is why I'm still interested in setData being returned, and I'm wondering what the issue with that is. I definitely understand being hesitant, because I am as well, but I'm not imagining a problem. Here is a different possible version of the syntax that could allow for less boilerplate. Notice the onMount, and the new opts argument. Maybe the opts is clumsy, and another method that only returned the data without setting it would be nicer, but I think it gets the point across.
With that said, I am understanding your recent todo examples and I am satisfied without the setData being returned. Even if returning setData is a good idea, there is no rush to implement this as far as I'm concerned. Btw, I like the proxy syntax, but not enough to give up destructuring syntax. But, what would ever be the use of |
So, the problem is that you need to |
Well, you are correct, but I'm not going to let that deter me. Regarding the 1,2,3 (try-catch-finally), you could easily cut that to 1,2 with two useReducer dispatches instead of 3 useStates sets, even without considering my previous ill-conceived suggestion of returning a dispatch, and it could appear to the user with the same syntax as now. I think it would cut down on a render, unless I misunderstand how it works. I think you could also return a setDataOnly method (that takes a function) that works exactly like setData from useState, but it could interact with the reducer. Or not. Either way, one less render. There is still the uncertainty about whether loading gets set to false before or after, but this is how I consider both scenarios with regards to putting
Arguably, an app should not rely on the 1,2,3 order, but if I understand, this is potentially the situation to be avoided, and in fact the situation I would expect, since you are returning data after
Somehow If I am not understanding things correctly, this will at least demonstrate my misunderstanding, and perhaps misunderstandings others may have. Anyway, I'll back off with this now. Everything seems to be moving very fast and I'm not solid with typescript. It's easy enough to read, but I always get some error that slows me down. No need to respond if I'm wrong. I'll just keep watching the issues and check out the documentation. Thanks. |
i think returning a dispatch is the wrong thing to do. it breaks
encapsulation and seems unnecessary.
On Sun, 7 Jul 2019 at 00:10, Chris Ellis ***@***.***> wrote:
Well, you are correct, but I'm not going to let that deter me.
Regarding the 1,2,3 (try-catch-finally), you could easily cut that to 1,2
with two useReducer dispatches instead of 3 useStates sets, even without
considering my previous ill-conceived suggestion of returning a dispatch,
and it could appear to the user with the same syntax as now. I think it
would cut down on a render, unless I misunderstand how it works. I think
you could also return a setDataOnly method (that takes a function) that
works exactly like setData from useState, but it could interact with the
reducer. Or not. Either way, one less render.
There is still the uncertainty about whether loading gets set to false
before or after, but this is how I consider both scenarios with regards to
putting request.loading = false in the todos example.
1. loading set to false *before* setTodos is called.
Arguably, an app should not rely on the 1,2,3 order, but if I understand,
this is potentially the situation to be avoided, and in fact the situation
I would expect, since you are returning data after finally { ... }. I
guess this could be bad if a render occurs before todos are ready, but why
set loading = false. It is already false. What good would that do?
1. loading set to false *after* setTodos is called.
Somehow setTodos gets called while loading is still true. Perhaps,
briefly, a spinner or loading indicator instead of a glorious list of
todos. Todos are still there in the data, and will get rendered because
when loading is eventually set to false.
If I am not understanding things correctly, this will at least demonstrate
my misunderstanding, and perhaps misunderstandings others may have.
Anyway, I'll back off with this now. Everything seems to be moving very
fast and I'm not solid with typescript. It's easy enough to read, but I
always get some error that slows me down. No need to respond if I'm wrong.
I'll just keep watching the issues and check out the documentation. Thanks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAA44OHKES7TABXUKSMZHYTP6EJ6LANCNFSM4HIY7VCA>
.
--
Cheers
Paul Cowan
Cutting-Edge Solutions (Scotland)
blog: http://thesoftwaresimpleton.com/
website: https://cutting.scot/
|
@dagda1 Yep, returning dispatch was "ill-conceived." Does setDataOnly have the same problems? |
in my opinion yes, you might as well create the hook yourself. i think i like the reducer idea. i would allow a transform function to be passed with the initial options that could be applied to any dataset once the remote resolves. |
Hi @alex-cory - Is there any way to "reset" the request? IE: Clear the 'error' in order to fire it again? I've a situation where I may want to fire a request a number of times (incorrect data on first try, then rectify and try again), and I'd like to be able to clear the "error" object somehow. I can get around it in other javascripty ways but was wondering if it's possible with the library. Similar to abort(), I guess, but more of a reset() function? |
Whenever you fire the request again, it automatically resets the error to const App = () => {
// assuming we already have a Provider with the `url` set in it
const { error, get, reset } = useFetch({ onMount: true, data: [] })
const doSomething = async () => {
await get() // <- this resets the error everytime it's called
reset() // <- just trying to see the scenario in which we need it! 😊
// is this kind of what you're talking about?
const data = await get()
if (!data) { // data is not what you think or want
reset()
await doSomething()
}
}
return (
<>
{error && error.message}
<button onClick={doSomething}>Do Something</button>
</>
)
} |
I have a feature request idea, it's something I am using in my own useFetch implementation, but with time my use cases expanded, and I need to switch to something like use-http. But I don't see that this lib has what I had, and that is a skip function/option, that just ignores the GET requests. The way I work is I have my state defined in the beginning as null or undefined (something falsey) and in the beginning, I don't want to make that requests because my data is not defined, as soon as an user interacts with a field (ex: search box) and starts typing, we start executing those GET requests. NOTE: the search box value is a part of the url, or queryParams, and based on that the hooks knows to refetch by itself Another one is useMock, which I use for when my endpoint/DB is slow, and I know the result I would get, it is always the same, so I start directly loading my "data", and avoid any requests. So when the backend guys fix that long query or that 500 error response, then we just switch off useMock prop to false and the fetch works normally.
Are there any similar replacements for these features, does it make sense to have them as part of useFetch at all? |
const { data, get } = useFetch('endpoint' + searchBox)
useEffect(() => {
if (searchBox) get()
}, [searchBox])
const { data, error, loading } = useFetch('endpoint', {
data: mockData // set's the default data for this as the mock data
}) |
Abort
I've been toying around with different ways to implement this.
Ideas:
We can abort multiple http requests at once
Individual Aborts
Typeahead usage
these are potential options for, if request1 is pending, and request2 is made, cancel request1. Or if they were set to 2, make 2 requests before canceling prior ones. Not sure the best way to handle this.
Syntax
I'm starting to like this syntax better, but what do you all think?
The text was updated successfully, but these errors were encountered: