Adds unit tests to Catalog actions #875
Conversation
|
This pull request is automatically deployed with Now. |
| location, | ||
| queryParameter: 'page' | ||
| }) | ||
| Math.floor( |
There was a problem hiding this comment.
This change is completely unrelated to the rest of this PR but since I had to spend time looking up what the ~~ operator does, I thought I'd save all future readers the effort.
There was a problem hiding this comment.
It is faster. Maybe a comment instead :)
There was a problem hiding this comment.
You looked it up and now you know what it does. No need to change it. 👍
There was a problem hiding this comment.
So I dug a little deeper and wrote up a little something.
Turns out that ~~ is not a straight replacement for Math.floor, specifically for the case of negative floating point numbers.
So on one hand since we're pulling our input from the query string, we can't be certain that it will be a positive number. But on the other hand, negative page numbers don't make sense (let alone negative floating point page numbers) so I don't think we care about them.
So I reverted this change and added a comment, as @sirugh suggested (see c6e1272).
I do want to voice my opinion though, that we should strongly consider readability when we write our code - especially in Venia, which is supposed to be a reference implementation.
Is ~~ faster? Sure. Is it more concise? Yeah. Do either of those things matter here? I'd argue that they do not.
If ~~ was a widely understood and commonly used paradigm in JavaScript I'd have less of a problem, but my gut tells me that the majority of the people who run across and try to understand this code are going to have to look up double bitwise NOT, and we can make this much more developer friendly.
| @@ -18,7 +18,6 @@ export const getAllCategories = () => | |||
| export const setCurrentPage = payload => | |||
| async function thunk(dispatch) { | |||
| dispatch(actions.setCurrentPage.receive(payload)); | |||
| window.scrollTo(0, 0); | |||
There was a problem hiding this comment.
Scrolling the window is a concern of the UI, not the action handler. See the corresponding change in category.js.
There was a problem hiding this comment.
Having window.scrollTo() in this particular action was a bit arbitrary—it only covered this one use case—but I don't actually think there's anything wrong with it being part of an async action. Ultimately, async actions are a natural place for side effects to live, and scrolling the window is just a side effect.
| location, | ||
| queryParameter: 'page' | ||
| }) | ||
| Math.floor( |
There was a problem hiding this comment.
You looked it up and now you know what it does. No need to change it. 👍
Good catch, thanks for bringing that up - I'm actually not sure how to force the error case. Here's the code: try {
// TODO: implement rest or graphql call for categories
// `/rest/V1/categories` requires auth for some reason
const { default: payload } = await import('./mockData');
dispatch(actions.getAllCategories.receive(payload));
} catch (error) {
dispatch(actions.getAllCategories.receive(error));
}First, I think it makes more sense to come back and write a test for this path once we get a real implementation in there, and second, I legit don't know how to mock / force an error to be thrown there 😝 . |
|
I'm good with that answer @supernova-at. |
Description
This PR adds unit tests to the Catalog's
actions.jsandasyncActions.js.Related Issue
Closes #873 .
Motivation and Context
This PR is a part of the larger effort to increase unit test coverage in PWA Studio.
How Has This Been Tested?
yarn test.Screenshots (if appropriate):
Proposed Labels for Change Type/Package
TEST
venia-concept.Checklist: