Skip to content
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

Order Client Loading States #256

Merged
merged 17 commits into from
Sep 28, 2023
Merged

Conversation

martijnvdbrug
Copy link
Member

@martijnvdbrug martijnvdbrug commented Sep 13, 2023

Description

This PR aims to implement global loading and error states for the active order store and the current user store.
Vue example:

<template>
  <div>
    <h1 v-if="loading">Loading...</h1>
    <h1 v-if="error">Something went wrong!</h1>
    <h1 v-if="data">Active order: {{ data.code }}</h1>
  </div>
</template>
<script setup lang="ts">
  import { VendureOrderClient } from "vendure-order-client";
  import { useStore } from "@nanostores/vue";

  const client = new VendureOrderClient(
    "http://localhost:3050/shop-api",
    "your-channel-token"
  );

  const { data, loading, error } = useStore(client.$activeOrder);
  await client.getActiveOrder();
</script>

⚠️ Still TODO:

  • For queries and mutations that handle activeOrders (like addItemToOrder, applyCouponCode, etc), you can use the decorator @HandleLoadingState('$activeOrder') to handle loading and error states for $activeorder
  • For queries and mutations that handle currentUser (like login), you can use @HandleLoadingState('$currentUser') so that it handles loading and error states for $currentUser
  • For each function/mutation:
    • Add the decorator @HandleLoadingState('$currentUser') or @HandleLoadingState('$activeOrder')
    • Use setResult(this.$activeOrder, activeOrder); or setResult(this.$currentUser, currentUser); to store the result (or ErrorResult) in the store.
    • Checkout getActiveOrder() as example, that should already be correctly implemented (untested)
    • And last, do this.throwIfErrorResult(activeOrder as ActiveOrder<A>); to return either an ActiveOrder or throw if an ErrorResult was returned.
  • All validateCurrentUser and validateOrder can be replaced with throwIfErrorResult
  • Testing: @nanostores/vue needs to be installed, so that we can test if loading and result states are correctly set. I guess we can use const { data, loading, error } = useStore(orderClient.$activeOrder); in our tests.
    • Testing the error handling once, for example in by adding a non-existent item to order is good enough, no need to test error state for each function, because it might be hard to mock errors in some cases (like for getActiveOrder())
    • Each test should test for the correct loading and data result though

⚠️ All of this is untested, so please let me know if you run into issues or if things aren't clear.

Breaking changes

  • Consumers can now destructure the store to get data, loading and error states:
import { useStore } from '@nanostores/vue'

- const activeOrder = useStore(orderClient.activeOrder);
+ const { data, loading, error } = useStore(orderClient.$activeOrder);

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains only a single feature. (Split into multiple PR's if needed)
  • I have reviewed my own PR, fixed all typo's and removed unused/commented code

⚡ Most of the time:

  • I have added or updated test cases for important functionality
  • I have updated the README if needed

📦 For publishable packages:

@dalyathan dalyathan marked this pull request as ready for review September 19, 2023 12:56
@dalyathan dalyathan changed the title Feat/order client loading states Order Client Loading States Sep 19, 2023
@martijnvdbrug
Copy link
Member Author

Nice work @dalyathan, thanks

Comment on lines 146 to 150
} else {
if (wasSetToTrue) {
wasFinallySetToFalse = true;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

These else statements could be simplified into:

Suggested change
} else {
if (wasSetToTrue) {
wasFinallySetToFalse = true;
}
}
} else if (wasSetToTrue) {
wasFinallySetToFalse = true;
}

BUT, another option would be to not use listenKeys at all:

      expect(client.$activeOrder.value?.loading).toBe(false);
      const promise = client.removeOrderLine('T_1');
      expect(client.$activeOrder.value?.loading).toBe(true);
      await promise;
      expect(client.$activeOrder.value?.loading).toBe(false);

It does require a bit of work to refactor, but I do find the code a bit more followable. I leave it up to you, both approaches are ok, but if you choose the listenKeys options we need to simplify the if/else statement like above.

I do like the custom expect messages, I was actually looking for something like this the other day, didnt know this worked 👐

Copy link
Member

Choose a reason for hiding this comment

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

Which is an even better approach. Updating PR now

Copy link
Member Author

@martijnvdbrug martijnvdbrug left a comment

Choose a reason for hiding this comment

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

Some changes required!

Copy link
Member Author

@martijnvdbrug martijnvdbrug left a comment

Choose a reason for hiding this comment

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

@dalyathan Nice, good idea with the helper function, nice and readable now!

@martijnvdbrug martijnvdbrug merged commit 32ccac7 into main Sep 28, 2023
26 checks passed
@martijnvdbrug martijnvdbrug deleted the feat/order-client-loading-states branch September 28, 2023 11:10
@martijnvdbrug martijnvdbrug restored the feat/order-client-loading-states branch September 28, 2023 11:29
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.

2 participants