Add a Customer Account API Abstraction

[blittle]

WHY are these changes introduced?

Implements the RFC here: #1189

WHAT is this pull request doing?

Implement the RFC and update the example to reference the new exposed functionality.

HOW to test your changes?

Follow the instructions in the Customer API example README.md

Post-merge steps

Checklist

• I've read the Contributing Guidelines
• I've considered possible cross-platform impacts (Mac, Linux, Windows)
• I've added a changeset if this PR contains user-facing or noteworthy changes
• I've added tests to cover my changes
• I've added or updated the documentation

[blittle]

This is the biggest thing that sucks about the abstraction at the moment. Any loader that calls the customer account api client must return the committed session. The only alternative I can think of is to always commit the session inside server.ts.

[frandiox]

Would it be a problem to always commit the session in server.ts? Would it make sense to do that automatically in the remix-oxygen adapter if you pass session?

[blittle]

Someone brought up a big downside of doing so. I'm trying to remember what the concern was. 🤔

[frandiox]

Could we change HydrogenSession to actually track changes somehow so we know if it's "dirty"?

Something like this:

class HydrogenSession {
// ...

  #isDirty = false;

  get isDirty() {
    return this.#isDirty;
  }

  set(...args) {
    this.#session.set(...args);
    this.#isDirty = true;
  }

  async commit() {
    const result = await this.#sessionStorage.commitSession(this.#session);
    this.#isDirty = false;
    return result;
  }

}

Then check session.isDirty before committing in server.ts / remix-oxygen? 🤔

[blittle]

I implemented this out, and after a discussion with @juanpprieto, I don't think I feel good about it for now. I'd rather just require the user to manually commit the session for now. Though, if I can convince the API folks to differentiate between these two scenarios, I think I might be able to warn the user they didn't persist/commit their session:

1. Try to refresh the auth token, but the refresh token is expired
2. Try to refresh the auth token, but the refresh token has already been used

[blittle]

It's super easy to get something small wrong in the oauth flow. In dev mode, I figured we can add these helper messages, potentially with links to a troubleshooting guide.

[blittle]

Maybe I'll pull this out and make it more obvious that this is a magic id.

[frandiox]

Now that we actually depend on this internally... should we also abstract away the HydrogenSession class that lives in userland's server.ts? Or at least provide some sort of abstract class that they can extend?

Otherwise it might be confusing to have this interface with the same name we have in the templates 🤔

[blittle]

Yeah, I wasn't sure what to do about that. I imagine most people take our default cookie session implementation, but I'm willing to bet other people use an actual DB of some sort to store sessions 🤔

[frandiox]

They could still extend our class and overwrite the constructor / etc. behavior? 🤔
Ah, maybe we wouldn't tree-shake the cookie related stuff in that case? So perhaps better to have an abstract HydrogenSession class and another HydrogenCookieSession? Well, just random ideas.

[blittle]

I'd like for people to be able to use any of the existing remix session utils easily. But it's looking like Remix 3 will be more opinionated about sessions. Maybe we should also be more opinionated about sessions? It would actually be pretty cool if we offered a default non-cookie based solution. For down the road though.

[frandiox]

Just to be consistent, should we wrap variables in an object like in the SFAPI? client.query('...', {variables: {}}).

Another thing we do in the SFAPI client is to check with a regex if you are actually passing a mutation to mutate or throw error. Not sure if we want to do the same here?

[blittle]

Should we be following this error standard everywhere? throw new Error('[h2:error:storefront.query] Cannot execute mutations');

[blittle]

I updated it to follow the same pattern, just [h2:error:customer.query]

[frandiox]

Oh yes, that's good, the CLI will pick it up 👍

[frandiox]

Would it be a problem to always commit the session in server.ts? Would it make sense to do that automatically in the remix-oxygen adapter if you pass session?

[frandiox]

Interesting, I hadn't considered this way of testing 🤔

[blittle]

Yeah, it's a bit obnoxious because Remix allows you to throw new Response but vitest uses uses check-error that expects everything to be an error with a .message property: https://github.com/chaijs/check-error/blob/main/index.js#L62

[frandiox]

@blittle I've fixed a couple of issues I found while reviewing, hope it doesn't break anything 😅