-
Notifications
You must be signed in to change notification settings - Fork 674
Conversation
Deploy preview for saleor-storefront-stage processing. Building with commit 6a65b66 https://app.netlify.com/sites/saleor-storefront-stage/deploys/5ece620d6df87f0006a9bd06 |
d66f4f7
to
0e33cd0
Compare
2e7e15c
to
54977d5
Compare
aab52c4
to
4bd57cd
Compare
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.
Besides one comment LGTM
add5d02
to
ef92ffe
Compare
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.
LGTM.
@dominik-zeglen please confirm if requested changes are fine
// setPromoCode?: boolean; | ||
// }; | ||
// } | ||
export type IJobsModel = Record<string, Record<string, boolean>>; |
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.
Why untyping it?
I wonder if it wouldn't be better (it'll be certainly easier) if we'd drop one level of nesting, simplifying this type to Record<"setCartItem" | "setPromoCode", boolean>
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, it might be typed, but it is not easy thing to do for me as it is not simple typing - I wrote string, because I didn't want to always redefine this type to be consistent with available methods in job classes.
And I'm not sure if dropping one level would good idea, because, what if we will have the same method name in two different job classes?
I want to merge this change because it changes checkout error handling.
Additionally, to simplify sdk, I have changed names from CheckoutRepositoryManager to LocalStorageManager and from NetworkManager to ApolloClientManager.
Screenshots
Pull Request Checklist