fix(venia): rm staleGuestCartId on error #354
Conversation
zetlen
requested review from
mhhansen,
pcvonz,
Crifen,
Starotitorov,
Serunde,
artKozinets and
JStein92
| * aramis: true | ||
| * }; | ||
| * | ||
| * console.lot(omit(musketeers, 'athos')); |
There was a problem hiding this comment.
I believe it should be console.log()
| module.exports = (obj, omitKey) => | ||
| Object.entries(obj).reduce( | ||
| (out, [key, value]) => | ||
| key === omitKey ? out : ((out[key] = value), out), |
There was a problem hiding this comment.
I propose to follow functional paradigm and do not mutate function arguments.
(out, [key, value]) => key === omitKey ? out : { ...out, [key]: value }
There was a problem hiding this comment.
In this implementation, the spread operator creates a new object and does not mutate the existing one.
However, there are other places where we do mutate intermediate results during a reduce operation. I consider these acceptable because they are local mutations only, to an object which is not exposed to the outside closure.
| * } | ||
| */ | ||
|
|
||
| module.exports = (obj, omitKey) => |
There was a problem hiding this comment.
In general, James, are you sure that we need to add custom implementation of omit function to the project? My personal opinion, that it will be better to add lodash or underscore to the project. This libraries contains a lot of useful methods, that we will need in our app in the future like omit. If we decide to write our own implementations of such methods, than we will have to maintain our code, cover it with tests, write docs etc. Lodash is very popular library. The only thing we should remember is to avoid imports import _ from 'lodash'; and just write, for example, import omit from 'lodash/omit';
There was a problem hiding this comment.
By the way, it will be useful to add ability to pass an array of omitKeys to the function.
There was a problem hiding this comment.
In my experience, Lodash adds a lot of file size, even when using tree-shaking. There are a few places in Venia and Peregrine where a Lodash method might be useful, but so far we have avoided its use. But I could be persuaded!
If you think that Lodash would ultimately help the codebase, then I would love for you to create a branch, refactor those parts of Venia and Peregrine to use Lodash, make sure they do not increase bundle size, and open a pull request!
|
@Starotitorov We may still decide to use Lodash in a few places, because you're right that we need to maintain as little code as possible. But for this PR, I realized that the standard in other reducers was to use an ES7 object spread and set the property to null, so I changed it to that. There's no longer any need for an omit function! Please review again when you have time. |
|
will double check in short to see if #344 stills reproduce |
|
looks good! 👍 |
Fixes #344.
The error was caused by a stale guestCartId whose corresponding cart in the API was no longer available. Since we don't have GraphQL for sessions yet, the guestCartId is stored in localStorage as well as in the Redux store. We were clearing it from localStorage on error, but forgetting to clear it from the store itself.
This fixes that!
This PR is a:
[ ] New feature
[ ] Enhancement/Optimization
[ ] Refactor
[x] Bugfix
[ ] Test for existing code
[ ] Documentation