-
Notifications
You must be signed in to change notification settings - Fork 34
wp-source: better api docs #141
Conversation
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.
Great job! 👏
I've added some comments below. I'm aware that some of the errors are previous to this PR, but let's take this opportunity to improve them 🙂
@@ -237,18 +274,18 @@ This action fetches all entities related to a `link`, i.e. the pathname of a URL | |||
All received data are populated in `state.source` and are accessible using the methods explained in the next section. | |||
|
|||
```javascript | |||
actions.source.fetch("/category/nature/"); | |||
actions.source.fetch("/category/nature/").then(dataFetched => ...) |
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.
Actions don't return data. Data is always accessed via the state
.
That's because Frontity is following the Flux pattern, like Redux.
|
||
```javascript | ||
const { api } = libraries.source; | ||
|
||
// Get posts from categories 2, 3 and 4 | ||
api.get({ endpoint: "posts", params: { _embed: true, categories: '2,3,4' } }); | ||
api.get({ endpoint: "posts", params: { _embed: true, categories: '2,3,4' } }) | ||
.then(dataRequested => { /* do something with dataRequested */ }) |
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.
Using promises with .then
is pretty much deprecated in favor of async/await nowadays. I'd rather show examples using await
:
const response = await api.get({
endpoint: "posts",
params: { _embed: true, categories: "2,3,4" },
});
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.
Using promises with .then is pretty much deprecated in favor of async/await nowadays. I'd rather show examples using await:
I'm not sure about this.
- I don't agree with the affirmation than the use of
.then
is deprecated - The use of
await
alone doesn't work unless you use inside of anasync
function
Using then
in the examples makes them ready to be "copied & pasted" while still indicates the nature of a Promise
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.
By deprecated I mean it's not used any more thanks to the async/await functions.
We (you and me) are used to it because we had to learn it before async/await existed. But for JS newbies the .then
prop is really confusing. If you think about it, it's quite a horrible API compared to async/await.
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're right 👍
* _`api`_: `string` _(optional)_ Overrides the value set with `api.set.` | ||
* _`isWpCom`_: `boolean` _(optional)_ Overrides the value set with `api.set.` | ||
- **Return value** | ||
- `Promise` Promise resolving to data requested |
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 the return value should be simply Promise
. It needs to contain a reference to the value that the promise resolves to, which is the useful information for the developer.
In this case the promise resolves to a Response
object from fetch
: https://developer.mozilla.org/en-US/docs/Web/API/Response
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've added a couple of details but it looks perfect to me 🙂
|
||
// Other endpoints: | ||
api.get({ | ||
const postBeautiesGullfoss = await api.get({ | ||
endpoint: "/frontity/v1/discovery", |
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.
This endpoint doesn't exist anymore because it belongs to our old plugin which is now deprecated. I don't know why this example is here, I guess we added it at the very beginning.
I would use another example, something that is used today. For example, the /acf/v3/posts
endpoint.
Actions don't return data. Data is always accessed via the state. | ||
That's because Frontity is following the [Flux pattern](https://facebook.github.io/flux/) (like Redux). |
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.
Maybe this information should be in the "Learning Frontity - Actions" section?
Apart from that, maybe it's worth mentioning that, even though actions don't return data, they return a promise that resolves when the action is finished.
You can do something like this:
await actions.source.fetch("/some-post");
which is useful when you need to access the new state just after calling the action:
await actions.source.fetch("/some-post"); // <- Wait until we fetch "/some-post".
const somePost = state.source.get("/some-post"); // <- The data will exist.
This is of course not needed on React components, because:
- They are not async (you cannot use
await
). - They re-render when the
state
accessed changes.
const SomePost = ({ actions, state }) => {
useEffect(async () => {
// You can use await here if you pass an async function, but it's not required.
await actions.source.fetch("/some-post");
}, []);
// The data will not exist at first, `data.isReady` will be false.
// But then, it will rerender when `actions.source.fetch` is finished.
const somePost = state.source.get("/some-post");
// This will work just fine.
return somePost.isReady ? <Post link="/some-post" /> : <Loading />;
};
What do you think?
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.
Nice 🙂
I don't know if the /acf/v3/posts
endpoint accepts a slug
parameter but I guess it's not important.
I've added an additional commit to replace route
with link
in all the references where route
was deprecated. I don't understand why we forgot to update this when we did the change.
The commit introduced a lot of format changes due to prettier
. Let's discuss how to solve that in https://community.frontity.org/t/code-documentation-workflow/1633.
Added API docs for every method and Table of Contents