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

Sirgalleto/add location modal for order routing extensions #1472

Merged

Conversation

sirgalleto
Copy link
Contributor

@sirgalleto sirgalleto commented Oct 26, 2023

Background

Add a new extension target for OrderRouting admin.settings.order-routing-rule.render and its LocationList component.

Solution

  1. Create a new remote component definition for the LocationList and exclude it from AllComponents since it should only be allowed in the OrderRouting extension target. Add docs + examples.
  2. Create a new target with AllComponents + OrderRoutingComponents available

🎩

Tested with our UI extension

Screenshot 2023-10-26 at 10 18 07 p m

Checklist

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

@sirgalleto sirgalleto force-pushed the sirgalleto/add-location-modal-for-order-routing-extensions branch from 63f166a to f1bfe0a Compare October 26, 2023 07:24
@sirgalleto sirgalleto force-pushed the sirgalleto/add-location-modal-for-order-routing-extensions branch from f1bfe0a to 7ccacb5 Compare October 26, 2023 07:42
@sirgalleto sirgalleto marked this pull request as ready for review October 26, 2023 07:47
@github-actions

This comment has been minimized.

Copy link
Member

@jbalsas jbalsas left a comment

Choose a reason for hiding this comment

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

Didn't 🎩 (is this something we could do through yarn link or yalc, perhaps?), but code makes sense to me!

@sirgalleto sirgalleto force-pushed the sirgalleto/add-location-modal-for-order-routing-extensions branch 3 times, most recently from 6da72d4 to 162cadf Compare November 2, 2023 17:24
@@ -0,0 +1,20 @@
import {ReferenceEntityTemplateSchema} from '@shopify/generate-docs';
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these docs ready to ship? Once this change gets merged in to the unstable branch, someone might generate the docs and cause this API to be published.

Copy link
Member

Choose a reason for hiding this comment

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

Not at all 😓

Copy link
Contributor

Choose a reason for hiding this comment

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

We should just remove these docfiles until we're ready to publish

Copy link
Member

Choose a reason for hiding this comment

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

Will do! I think we weren't super sure what was the process here so this was a bit of a shot in the dark. If I understand correctly, we should:

  • Commit the types so they get to unstable
  • Work on the docs separately and merge them when ready

Would that be the expected workflow?

Is there a timeline as to when unstable becomes stable? Is there any sort of deadline we'd want to hit?

extends StandardApi<ExtensionTarget> {
/* Utilities for translating content according to the current `localization` of the admin. */
i18n: I18n;
data: {
Copy link
Member

Choose a reason for hiding this comment

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

We'll also need to bring in saveMetafields from https://github.com/Shopify/web/pull/108366 once ready.

@olavoasantos, @MitchLillie, could you walk me through how would we accomplish that? Is this supposed to be augmented somewhere else for the plugins?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you just need to add whatever methods/properties that plugin provides developers to the typedef here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, makes sense... @olavoasantos suggested (or so I understood) typing them here and then consuming the types in web from here. So I'll add that.

cc @cpeddecord , would you say our types are solid enough to get them over here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer for that PR to land before moving the types over, but otherwise I'm confident the public API won't change

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, makes sense, I'll just prep over here but wait for it to land before pushing this!!

Copy link
Member

Choose a reason for hiding this comment

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

PR should be good to go now, waiting on https://github.com/Shopify/web/pull/108366/files and a potential ✅ from @MitchLillie here before going forward with this.

@jbalsas jbalsas force-pushed the sirgalleto/add-location-modal-for-order-routing-extensions branch from 162cadf to c85cc52 Compare November 9, 2023 12:22
@jbalsas jbalsas requested a review from MitchLillie November 9, 2023 13:17
'list.weight',
] as const;

interface MetafieldChange {
Copy link
Member

@jbalsas jbalsas Nov 9, 2023

Choose a reason for hiding this comment

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

@olavoasantos, @MitchLillie, there's still no easy way to expose this to the extensions (nor to web, for that matter).

I managed to resolve the type through some TS gymnastics like:

type ExtractElementType<T> = T extends readonly (infer ElementType)[]
  ? ElementType
  : never;

type ApiForRenderExtension = ReturnType<
  typeof useApi<'admin.settings.order-routing-rule.render'>
>;

type ExtractMethod<T extends keyof ApiForRenderExtension> =
  ApiForRenderExtension[T];

type SaveMetafields = ExtractMethod<'saveMetafields'>;

type MetafieldChange = ExtractElementType<Parameters<SaveMetafields>[0]>;

I like how the types are clean and properly derived from useApi(TARGET), but wondering if we'd want to expose some of the helpers that already exist in this repo as well in order to make this slightly simpler... or to allow for some sort of additional types 🤔

Normally one wouldn't need this, but as things grow, types might be needed in files that do not call useApi making some of this necessary 🤔

Copy link
Contributor

@MitchLillie MitchLillie Nov 9, 2023

Choose a reason for hiding this comment

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

I like how the types are clean and properly derived from useApi(TARGET)

Couldn't you just export and import these types directly? For example you're exporting LocationProps already. I know this would make them not-perfectly derived from useApi, but it would make the extension code simpler.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but it would be the only ones... we're currently not exporting anything anything else, not even the ApiForRenderExtension and other useful types that already exist, to I don't see how it would make sense to directly export such a dedicated type.

It might be fine in most cases, so I'm fine with the ocasional additional complexity for now, just flagging it so we can keep this in the radar for future improvements!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, makes sense. This would be a good enhancement. I created #1525

@cpeddecord
Copy link
Contributor

can we hold on merging this? We may need to update the saveMetafieldsPlugin's public API to match checkout-web's

https://shopify.slack.com/archives/C04710CKFMH/p1699642277565379

@jbalsas
Copy link
Member

jbalsas commented Nov 13, 2023

can we hold on merging this? We may need to update the saveMetafieldsPlugin's public API to match checkout-web's

Sure thing @cpeddecord, please just signal when ready!

Copy link
Contributor

@MitchLillie MitchLillie left a comment

Choose a reason for hiding this comment

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

Didn't 🎩, but code looks good. It would be good to have someone 🎩 before merge, I can take a look a bit later today.

Note: Please run yarn changeset as suggested in this comment. It's required to generate our changelog properly.

@sirgalleto sirgalleto force-pushed the sirgalleto/add-location-modal-for-order-routing-extensions branch from 1242282 to 5319676 Compare November 15, 2023 09:49
@jbalsas jbalsas force-pushed the sirgalleto/add-location-modal-for-order-routing-extensions branch from b58d17b to 4bc256c Compare November 30, 2023 12:53
@jbalsas
Copy link
Member

jbalsas commented Nov 30, 2023

@MitchLillie, @sirgalleto, @cpeddecord, I've updated the types here and 🎩 with https://github.com/Shopify/order-routing-default-function-app/pull/199

Rebuilding the consumer seems to produce the right types:

Screenshot 2023-11-30 at 12 54 15

@jbalsas jbalsas force-pushed the sirgalleto/add-location-modal-for-order-routing-extensions branch from 4bc256c to 9c83655 Compare November 30, 2023 13:41
@jbalsas jbalsas merged commit 0841289 into unstable Nov 30, 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.

4 participants