Skip to content

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

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Jun 20, 2025

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:

  • Added editable property
  • Added useCartEditable hook
  • Added CartNotEditableError error class

This 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 the editable check or is that simply noise?

For example, the react examples would probably look like:

  const editable = useCartEditable();
  // ...
  return (
    <Tile 
      // ...
      onPress={() => editable && api.cart.setCustomer({
        id: 1,
      })}
    />
  );

And TypeScript examples would look like:

  const tile = root.createComponent(Tile, {
    // ...
    onPress: () => api.cart.subscribable.initial.editable && api.cart.setCustomer({
      id: 1,
    }),
  });
  // ...
 api.cart.subscribable.subscribe((newCart: Cart) => {
    tile.updateProps({
      onPress: () => editable && api.cart.setCustomer({
        id: 1,
      }),
    });
  });

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 modifying enabled instead of doing editable && ....

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:

API Docs Release Notes Example
Cart API Docs Release Notes Example

Checklist

  • I have 🎩'd these changes
  • I have updated relevant documentation

Copy link
Member Author

MrGVSV commented Jun 20, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@MrGVSV MrGVSV force-pushed the 06-20-update_docs_for_editable_property branch 2 times, most recently from 21d2b4a to ee37cff Compare June 20, 2025 20:56
@MrGVSV MrGVSV marked this pull request as ready for review June 20, 2025 21:07
@MrGVSV MrGVSV requested a review from vctrchu June 20, 2025 21:07
Copy link
Contributor

We detected some changes in packages/*/package.json or packages/*/src, and there are no updates in the .changeset directory. If the changes are user-facing and should cause a version bump, run yarn changeset to track your changes and include them in the next release CHANGELOG. If you are making simple updates to repo configuration, examples, or documentation, you do not need to add a changeset.

Copy link
Contributor

@vctrchu vctrchu left a 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?

Copy link
Member Author

MrGVSV commented Jun 20, 2025

What is the editable default value? true?

POS will default to true, yes. But to ease migration, we're also treating undefined as true as well.

@vctrchu
Copy link
Contributor

vctrchu commented Jun 20, 2025

POS will default to true, yes. But to ease migration, we're also treating undefined as true as well.

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.

@MrGVSV MrGVSV force-pushed the 06-20-update_docs_for_editable_property branch from ee37cff to d9acbef Compare June 20, 2025 23:40
@MrGVSV MrGVSV force-pushed the 06-20-update_docs_for_editable_property branch from d9acbef to e3b6687 Compare June 20, 2025 23:41
@MrGVSV MrGVSV requested a review from vctrchu June 20, 2025 23:44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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?

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

@lrsterrett
Copy link
Contributor

Was this doc change intended to be ready for 2025-07 stable release planned for today?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants