-
Notifications
You must be signed in to change notification settings - Fork 1
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
added: define fetch docs and quick reference examples #1
Conversation
| Prop | Type | Default | Description | | ||
| --------- | --------------------------------------------------------------------------- | ---------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| `key` | string[] | required\* | These strings will be used to create a key for the data store. | | ||
| `request` | func (...requestArgs)<br /> Returns -> <br /> func (...resiftServices) <br> | required\* | The `request` function is what will dispatch. The return data is what will be stored and returned in `useFetch`. <br /><br /> `requestArgs` are optional arguments needed for a request. <br /><br /> `resiftServices` are services such as `http` and `dispatch` supplied through Resift that give additional functionality to your request. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ricokahler, what are the standards on arguments like these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any arguments that are required to make a request but aren't required to identify the fetch should go in the request
args, otherwise (if they identify the request) then they should go in the make
args
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These docs are great. I was a bit strict on the examples because they'll serve a model for others to understand but other than that, they're great
docs/API/defineFetch-share.md
Outdated
| Prop | Type | Default | Description | | ||
| ----------- | ---------------- | ---------- | ------------------------------------------------------------------------------------------------------------------ | | ||
| `namespace` | string | required\* | Another `defineFetch` function sharing the same name space will result them sharing the same key to the data store | | ||
| `merge` | func(prev, next) | required\* | `merge` adds customizing functionality before the data is applied to the data store. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge isn't required (or at least isn't supposed to be). if merge isn't provided, the default behavior is that the next
will replace the previous e.g. merge: (_prev, next) => next
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update this.
docs/API/defineFetch-share.md
Outdated
share: { | ||
namespace: 'person', | ||
// Keep data flow as is | ||
merge: (previous, next) => ({ ...previous, ...next }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in context of the comment above, you could omit this logic. I think a good example of merge
would be to update a single person in the list after a PUT
, otherwise, I think it makes sense to prefer replacement instead of merging (i.e. (prev, next) => next
vs (prev, next) => ({...prev, ...next})
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
docs/API/defineFetch-share.md
Outdated
// Keep data flow as is | ||
merge: (previous, next) => ({ ...previous, ...next }), | ||
}, | ||
make: (personName: string) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have personName
here by personId
down there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
``` | ||
|
||
```tsx | ||
// Example container using `definefetch` above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the open source, docs, I don't think we should suggest the container pattern. it's not something I see in the community and is probably unique to us
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ricokahler, what do you recommend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to just call this component <Person />
or <Profile />
. Instead of putting this logic in a container component, let's just make the example put it in the component itself.
Unless they've used redux before, I just don't think people will know what a container component is. And from the mouth of the prophet himself:
Update from 2019: I wrote this article a long time ago and my views have since evolved. In particular, I don’t suggest splitting your components [into container and presentational components] anymore. If you find it natural in your codebase, this pattern can be handy. But I’ve seen it enforced without any necessity and with almost dogmatic fervor far too many times. The main reason I found it useful was because it let me separate complex stateful logic from other aspects of the component. Hooks let me do the same thing without an arbitrary division. This text is left intact for historical reasons but don’t take it too seriously.
docs/API/defineFetch-share.md
Outdated
dispatch(getPerson()); | ||
}, []); | ||
|
||
const handleEditPerson = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we suggest using useCallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed since it complicates things.
share: { | ||
namespace: 'person', | ||
// Keep data flow as is | ||
merge: (previous, next) => ({ ...previous, ...next }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I think it would be better to omit merge
to prefer the default merge behavior (replacement)
// Keep data flow as is | ||
merge: (previous, next) => ({ ...previous, ...next }), | ||
}, | ||
make: (personName: string) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant personId
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
namespace: 'person', | ||
merge: (previous, next) => { | ||
// Customize and add data | ||
return { ...previous, ...next, lastEdited: new Date() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a valid use case of merge. In this case, it would be better to add lastEdited
as part of the request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return { ...previous, ...next, lastEdited: new Date() } | ||
}, | ||
}, | ||
make: (personName: string) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above, i think you meant personId
?
const getPeople = makeGetPeople(); | ||
dispatch(getPeople()); | ||
|
||
return errors.length ? errors : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to return a list of errors here. In real usage, it might be better to throw if there are any errors.
Since this is an example, I think the easiest way to teach the concept is to just sequentially delete all the people using async
, await
, and a normal for of
loop.
If there is any error, it will cause the whole fetch to stop and error status to show up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JulesPatry I merged the API docs generator into #2 . Pull the latest from that branch and then you can update Run this command locally to get the API docs to generate. Unfortunately there is no hot reloading for the
|
I'm closing this because we changed how API documentation works since this PR was opened, however, I'll come back to this PR and copy and paste your docs to where they should be in the typings. I'll give you co-author too |
Created Docs for