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

set unstable to latest customer-account internal state #1448

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

robin-drexler
Copy link
Member

@robin-drexler robin-drexler commented Oct 18, 2023

Background

This PR moves back our internal apis to unstable in preparation for release.

It mainly ports over new components (like Card) and APIs that we have added, but also includes some type renamings caused by clashes in OrderStatus and StandardApi.

The PR also ensures that hooks that only work on the ODP properly throw errors if used elsewhere.

🎩

Here's what I did on the spinstance to make this 🎩 able.

inside ui-extensions

yarn build

To build the packages and make them consumable locally.

Then inside the customer account and extension repos

rm -rf node_modules/@shopify/{ui-extensions,ui-extensions-react} && cp -r ../ui-extensions/packages/{ui-extensions,ui-extensions-react} node_modules/@shopify/

So both are now using the locally built version of ui-extension packages.

I ran type-check inside in both repos, which succeeded.
It'll run again properly in CI after we merge this PR and update the version properly in package.json

You can see the still working extension here.

Checklist

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

@robin-drexler robin-drexler force-pushed the rd/customer-account-unstable branch 5 times, most recently from be84199 to 9c193e0 Compare October 19, 2023 12:49
@@ -50,10 +50,12 @@ export type {
MapLocation,
MaybeConditionalStyle,
MaybeResponsiveConditionalStyle,
IconSource,
Copy link
Member Author

Choose a reason for hiding this comment

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

Those seemed to be missing. Other non-prop exports of components , like MapBounds are exported here as well.

@@ -17,3 +17,13 @@ export class ExtensionHasNoMethodError extends Error {
);
}
}

export class ExtensionHasNoFieldError extends 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.

A bunch of our hooks only work on the ODP. We're throwing this error when they are used outside of the ODP.

This is done so that we can keep the typings that we have. Alternatively, we'd have to mark all of them as optional which would be a breaking change for existing OSP/ODP extensions.

@robin-drexler robin-drexler force-pushed the rd/customer-account-unstable branch 4 times, most recently from 6ef36dd to 6822b3d Compare October 19, 2023 19:35
@robin-drexler robin-drexler marked this pull request as ready for review October 19, 2023 19:54
@github-actions

This comment has been minimized.

export type {
AccessibilityRole,
ActionExtensionApi,
Copy link
Member Author

Choose a reason for hiding this comment

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

Added new APIs

Copy link
Contributor

@lsit lsit left a comment

Choose a reason for hiding this comment

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

Components and related types look good

@@ -5,14 +5,21 @@ import type {

import {useApi} from './api';
import {useSubscription} from './subscription';
import {ExtensionHasNoFieldError} from '../errors';

/**
* Returns the proposed `attributes` applied to the checkout.
*/
export function useAttributes<
Target extends RenderExtensionTarget = RenderExtensionTarget,
Copy link
Contributor

Choose a reason for hiding this comment

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

hm... we limited it to only RenderOrderStatusExtensionTarget to strongly suggest that it should only be used on order status page, why do we change it back to RenderExtensionTarget as it will not put any type check error for targets outside order status page ? https://github.com/Shopify/ui-extensions/blob/internal/packages/ui-extensions-react/src/surfaces/customer-account/hooks/attributes.ts#L24

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

adjusted it. Unfortunately, I had to update a bunch of tests to set extension.target in the api as it would've otherwise failed. It's actually better, but it makes the PR seem bigger

@robin-drexler robin-drexler force-pushed the rd/customer-account-unstable branch 2 times, most recently from 6b9a15a to dd85a78 Compare October 19, 2023 21:09
>(filters: AppMetafieldFilters = {}): AppMetafieldEntry[] {
const appMetafields = useSubscription(useApi<Target>().appMetafields);
const api = useApi<Target>();
const extensionTarget = api.extension.target;
Copy link
Contributor

Choose a reason for hiding this comment

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

if we switch https://github.com/Shopify/ui-extensions/pull/1448/files#diff-ccb0ccef13a6c51f7ca59868cf8bc5ef122d304f166692a758ea3e83abe796a3R24 error's constructor from
constructor(field: string, target: ExtensionTarget) {
to
constructor(field: string, target: RenderExtensionTarget) { will we be able to get rid of the extra const here?

Copy link
Member Author

@robin-drexler robin-drexler Oct 20, 2023

Choose a reason for hiding this comment

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

No, it has unfortunately nothing to do with the Error.

Take this example here:

export function useAppMetafields<
  Target extends RenderOrderStatusExtensionTarget = RenderOrderStatusExtensionTarget,
>(filters: AppMetafieldFilters = {}): AppMetafieldEntry[] {
  const api = useApi<Target>();
  const extensionTarget = api.extension.target;

  if (!('appMetafields' in api)) {
    // api will be `never` inside this if block. Even before we even use the `Error`
   // it's because it's typing-wise impossible to get into this state
   // you can't have no appMetafields and still have a valid `api` object. (technically)
  // but not everybode uses TS, so we still want to throw if someone uses this hook for an extension point that does not make sense
  
    throw new ExtensionHasNoFieldError('appMetafields', extensionTarget);
  }
}

Copy link
Contributor

@brianshen1990 brianshen1990 left a comment

Choose a reason for hiding this comment

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

tophatted and looks good, let's release a new unstable version to have a test~

@robin-drexler robin-drexler merged commit 8223c0b into unstable Oct 20, 2023
5 checks passed
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.

3 participants