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

Add editor extension collection spec #3551

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

oluwatimio
Copy link
Contributor

@oluwatimio oluwatimio commented Mar 12, 2024

WHY are these changes introduced?

Fixes https://github.com/Shopify/core-issues/issues/66051

Also check your spin branch out to

https://github.com/Shopify/shopify/pull/490149
https://github.com/Shopify/partners/pull/53863

WHAT is this pull request doing?

Adding new extension collection spec to the cli

How to test your changes?

To have an extension code locally clone Shopify/customer-accounts-ui-extension-dev and also clone CLI locally

  1. Clone CLI locally
  2. Run spin up customer-accounts-ui-extension-dev
  3. Checkout my branches in partners and shopify core
  4. Run dev refresh on both shopify core and partners in spin.
  5. Go to partners org internal, and for your org enable the beta flag 'editor_extension_collection' for example the link looks like https://partners.build-extension-collections.oluwatimi-owoturo.us.spin.dev/internal/organizations/1
Screenshot 2024-03-14 at 2 45 52 PM
  1. In the CLI run the following command
SHOPIFY_CLI_VERSIONED_APP_CONFIG=1 SHOPIFY_SERVICE_ENV=spin SPIN_INSTANCE=${your-spin-instance} pnpm shopify app generate extension --path ${pathToYourApp}

You should see

Screenshot 2024-03-15 at 11 38 05 AM
  1. Generate the Editor extension collection under the admin group
  2. Go to the extension in customer-accounts-ui-extension-dev
  3. You can generate another extension to reference the handle of that extension inside in_collection array. You can also try using the other method (extensions.in_collection) in the template to reference the extension handle and make sure both are not included at the same time.
  4. Try and deploy the extension collection by running
SHOPIFY_CLI_VERSIONED_APP_CONFIG=1 SHOPIFY_SERVICE_ENV=spin SPIN_INSTANCE=${your-spin-instance} pnpm shopify app deploy --path ${pathToYourApp}
  1. Click on the DB Icon in shopify core spin service and open Sequel Ace.
  2. Select shopify_dev as the db
  3. Search for the shopify_dev.app_static_extension_configs table
  4. You should see the extension collection config there
Screenshot 2024-03-15 at 11 34 46 AM

Post-release steps

This will not be visible in prod because it is behind a beta flag

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

Copy link
Contributor

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

  • Themes: @shopify/advanced-edits
  • UI extensions: @shopify/ui-extensions-cli
    • Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
  • Hydrogen: @shopify/hydrogen
  • Other: @shopify/cli-foundations

Copy link
Contributor

github-actions bot commented Mar 12, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.55% (+0.3% 🔼)
6642/9155
🟡 Branches
69.48% (+0.15% 🔼)
3217/4630
🟡 Functions
71.56% (+0.7% 🔼)
1756/2454
🟡 Lines
73.78% (+0.31% 🔼)
6266/8493
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / editor_extension_collection.ts
100% 100% 100% 100%
🟢
... / error-handler.ts
100% 100% 100% 100%
🟢
... / semver.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🔴
... / trigger.ts
0%
0% (-100% 🔻)
0% 0%
🟢
... / loader.ts
92.91% (-0.22% 🔻)
86.23% (-0.57% 🔻)
94.94% (-0.19% 🔻)
93.89% (-0.2% 🔻)
🔴
... / dev.ts
9.8% (-0.2% 🔻)
0% 17.65%
9.57% (-0.21% 🔻)
🟢
... / use.ts
96.55% (+0.12% 🔼)
94.44% (-0.29% 🔻)
100%
96.55% (+0.12% 🔼)
🟢
... / urls.ts
82.24% (-0.33% 🔻)
78.31% (-0.51% 🔻)
75%
84.16% (-0.31% 🔻)
🟢
... / find-app-info.ts
96.88% (-0.09% 🔻)
92.86% 100%
96.67% (-0.11% 🔻)
🟢
... / send-app-uninstalled-webhook.ts
85% (-0.71% 🔻)
75% 100%
85% (-0.71% 🔻)
🟢
... / ConcurrentOutput.tsx
97.62% (-2.38% 🔻)
75% (-8.33% 🔻)
100%
97.44% (-2.56% 🔻)
🟡
... / output.ts
71.79% (-0.47% 🔻)
67.69% (-0.96% 🔻)
63.64%
72.73% (-0.49% 🔻)
🟢
... / spin.ts
96.67% (-0.83% 🔻)
60% (-5.38% 🔻)
100%
96.55% (-0.88% 🔻)

Test suite run success

1590 tests passing in 744 suites.

Report generated by 🧪jest coverage report action from 3720a1e

@oluwatimio oluwatimio changed the title [WIP] Add editor extension collection spec Add editor extension collection spec Mar 15, 2024
@oluwatimio oluwatimio self-assigned this Mar 15, 2024
@oluwatimio oluwatimio marked this pull request as ready for review March 15, 2024 18:18
Copy link
Contributor

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

schema: EditorExtensionCollectionSchema,
appModuleFeatures: (_) => ['bundling'],
deployConfig: async (config, _) => {
if (config.in_collection && isArrayofStrings(config.in_collection)) {
Copy link
Contributor Author

@oluwatimio oluwatimio Mar 15, 2024

Choose a reason for hiding this comment

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

This may end up going in future. We are thinking of changing this property to includes and then we will also have extension.include so we will not need this logic anymore

const editorExtensionCollectionSpecification = createExtensionSpecification({
identifier: 'editor_extension_collection',
schema: EditorExtensionCollectionSchema,
appModuleFeatures: (_) => ['bundling'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

validation and localization to follow in other PRs

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 pretty sure you don't need bundling here, can you try to remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, just removed it! Thanks

handle: zod.string(),
})

export const EditorExtensionCollectionSchema = BaseSchema.extend({
in_collection: zod.union([zod.array(InCollectionSchema), zod.array(zod.string())]).optional(),
include: zod.array(IncludeSchema).optional(),
Copy link
Contributor Author

@oluwatimio oluwatimio Mar 19, 2024

Choose a reason for hiding this comment

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

Both optional so you can use either one of them without needing to declare the other. We will add validation to make sure at least one of them exists

load collection in cli

add tests

InCollection interface

remove check for is array

in_collection optional

remove bundling

update in collection logic

update to includes and include

update logic
@oluwatimio oluwatimio force-pushed the add-editor-extension-collection-spec branch from d89f8dd to 3720a1e Compare March 20, 2024 19:49
@oluwatimio oluwatimio added this pull request to the merge queue Mar 20, 2024
Merged via the queue into main with commit e29d4df Mar 20, 2024
@oluwatimio oluwatimio deleted the add-editor-extension-collection-spec branch March 20, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants