-
Notifications
You must be signed in to change notification settings - Fork 43
Update docs for editable
property
#3002
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
base: unstable
Are you sure you want to change the base?
Conversation
21d2b4a
to
ee37cff
Compare
We detected some changes in |
packages/ui-extensions/docs/surfaces/point-of-sale/staticPages/pages/versions.doc.ts
Outdated
Show resolved
Hide resolved
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.
What is the editable
default value? true?
POS will default to |
If the default is true I'd say its fine to not update all the examples since it won't affect existing implementations. A single example showcasing the specific editable and error should be sufficient. |
ee37cff
to
d9acbef
Compare
d9acbef
to
e3b6687
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.
@NathanJolly @js-goupil Thoughts on this? Every API call can possibly throw an editable error.
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.
@aaronschubert0 Do you have any thoughts?
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.
@MrGVSV If extensions don't directly handle CartNotEditableError
, how does it affect the app?
My preference would be that in the case of CartNotEditableError
we return an error from these methods rather than throwing an error.
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.
If extensions don't directly handle
CartNotEditableError
, how does it affect the app?
We have a tech doc that goes into more detail, but failing to handle the error would result in the default UI Extension error behavior (i.e. the extension enters an error state and/or a toast is displayed).
My preference would be that in the case of
CartNotEditableError
we return an error from these methods rather than throwing an error.
The reason we throw instead of following the error-as-value approach is that we want to reduce the overall changes introduced here. The idea is that this is just a stopgap until we can get certain other features that allow for a cleaner workflow.
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.
When we say reduce the overall changes introduced here, are speaking about the ui-extensions or POS? I think @aaronschubert0 is referring to throwing an error like this. Would this be too much work?
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.
Oh I think I misunderstood then. Yes the plan is to throw the CartNotEditableError
like that. I was thinking that the suggestion was to change the methods to return either a success or error state. That would be beyond the scope we want to introduce as part of our temporary solution here.
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.
Throwing a typed error is good. But should the error be more general? Using CartNotEditableError
for every method feels off. A general CartError
gives us more flexibility. If requirements change, we can add new error types without breaking things or needing a big refactor. 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.
Yeah that works. The benefit of CartNotEditableError
was that it would allow for more fine-grained and type-safe control over how users handle those errors. But we can loosen the type-safety and use a generic CartError
.
We actually aren't making use of any of these changes in POS yet (that should come in the next couple months), so we have the freedom to choose whatever type of error we throw.
I guess the one question I have is, if we did want to use CartError
, would we be able to add that in a patch release for 2025-07? Or is adding it a problem?
If it is a problem and we do want to avoid using CartNotEditableError
, we can also consider using a basic Error
for now.
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'm good with using a basic Error
for now as I think creating an CartError
type should require a separate PR and reasoning itself. Sorry for the back and forth, this should keep you building for the time being.
You can just remove the Throws
comments in the useCartApi
and we can merge the rest.
Was this doc change intended to be ready for 2025-07 stable release planned for today? |
Resolves https://github.com/shop/issues-retail/issues/8521
Background
The changes made in #2986 need their documentation to be updated.
Solution
Adds release notes for:
editable
propertyuseCartEditable
hookCartNotEditableError
error classThis PR also changes the API method docs to use
Throws
instead of@throws
as our internal documentation engine doesn't support those tags yet. Ideally, we'd be able to make use of such tags, but for now I think it makes sense to just do without them.Question
Now
editable
affects all cart methods, throwing an error when the cart is not editable. Should we update all examples to include theeditable
check or is that simply noise?For example, the react examples would probably look like:
And TypeScript examples would look like:
Note that the other way of handling cart editability is to disable the tile when
editable
is false. So we could also update the examples in a similar way but just modifyingenabled
instead of doingeditable && ...
.How do we feel about including these in the examples? Does it detract from the actual example too much?
🎩
https://pos.docs.shopify.io/extensions/contributing/documentation#how-to-update-docs-for-stable-api-versions
Here is the result:
Checklist