Add pos_ui_extension to migration for ui_extension #4162

js-goupil · 2024-07-04T16:25:49Z

WHY are these changes introduced?

We noticed that while testing a deploy for Stocky to the new ui extension spec, that the registrationUuid gets regenerated. This is problematic for smart grid tiles that are already existing, as they refer to these uuids in order to exist. If the registrationUuid no longer exists, then the tile will break

WHAT is this pull request doing?

Adds pos_ui_extension extension types to the migrator.

How to test your changes?

I have a separate branch that has a test extension commited. Check this branch out in my spin instance.
You'll need to create a new app by running shopify app init. You will also need an extension, but you can copy this folder and toss it into your new app. You can just copy this folder
sandbox-170.zip
into your local spin instance. Make sure you run npm install to install the dependencies.
Deploy the extension by doing pnpm shopify app deploy --path {path to your app} --verbose. This should deploy the extension in the old spec.
For convenience purposes, attach the app to the existing Extension Spin app, since it's already published on shop 1.
Go to graphQL for shop-1, and query with the specificationIdentifier: "pos_ui_extension" to make sure that it's there. Take note of the registrationUuid.
Change the toml file to the following:

name = "sandbox-170"
type = "ui_extension"
handle = "sandbox-170"
description = "sandbox-170"
api_version = "2024-04"

[[extension_points]]
target = "pos.home.tile.render"
module = "./src/index.tsx"

Run pnpm build and pnpm shopify app build.
Run pnpm shopify app deploy --path {path to your app} --verbose again. This time you should get a prompt to migrate your extension. Hit yes.
This should run succesfully, and prompt you to deploy a new version.
Once that's done, go to graphql for shop-1 and query with the specificationIdentifier: "ui_extension". You should see your extension there with the SAME registrationUuid.

Post-release steps

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

rtymchyk

Looks like this was used in the past & reads like it should do the right thing, so LGTM. Thank you!

vividviolet

Hmm CI failing could be a red herring. Can you update the tests and try to re-run locally? We need a new test for pos_ui_extension

github-actions · 2024-07-04T17:46:50Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	72.49% (-0% 🔻)	7547/10411
🟡	Branches	69.25% (+0.01% 🔼)	3718/5369
🟡	Functions	71.44% (-0.03% 🔻)	1996/2794
🟡	Lines	72.82% (+0% 🔼)	7127/9787

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / prompts.ts	31.58% (-3.72% 🔻)	0%	30% (-3.33% 🔻)	28.57% (-2.2% 🔻)

Test suite run success

1735 tests passing in 800 suites.

Report generated by 🧪jest coverage report action from cb06f10

js-goupil · 2024-07-04T18:02:19Z

@vividviolet good call out, turns out the issue was fixed by rebasing with main. Did add a test case though

vividviolet

Looks good to me. We should only merge after the Shopify PR gets shipped. Can you put up a PR? I'll top hat end-to-end with the other PR

js-goupil · 2024-07-04T18:15:25Z

@vividviolet yup, already had one in draft, but marked it as ready for review https://github.com/Shopify/shopify/pull/518353

Also not sure if it helps, but my spin instance linked above has both these cli changes and the core one

vividviolet · 2024-07-04T20:26:24Z

ps, but my spin instance linked above has both these cli changes and the core one

I don't think I can point to your spin unfortunately so I have to check out your branches on my own instance

isaacroldan · 2024-07-05T07:53:08Z

We have a release next week so it'll be ideal to have this merged today, let me know how the tophat goes @vividviolet 🙏

vividviolet · 2024-07-05T14:13:19Z

/snapit

isaacroldan · 2024-07-05T14:21:38Z

/snapit

github-actions · 2024-07-05T14:23:46Z

🫰✨ Thanks @isaacroldan! Your snapshot has been published to npm.

Test the snapshot by installing the package globally:

pnpm i -g @shopify/[email protected]

js-goupil · 2024-07-05T14:48:24Z

/snapit

I had no idea this was a thing, that's cool as hell

vividviolet

Tested end-to-end - everything looks good. Since you're making changes to the migrator, do you mind updating the message? It's a bit confusing because it doesn't say what we're migrating to:

Maybe it should be

Your "sandbox-170" configuration has been updated to a UI extension and needs to be migrated. Migrating gives you access to new features and won't impact the end user experience. All previous extension versions will reflect this change.

And then the prompt should be "Yes, confirm migration from "pos_ui_extension" to "ui_extension"?

isaacroldan · 2024-07-05T15:13:23Z

Agree on Trish's proposal 👌
Also, please run pnpm changeset add to add a patch changeset in this PR

js-goupil · 2024-07-05T17:20:15Z

.changeset/rare-lizards-heal.md

+'@shopify/cli': minor
+---
+
+Added extension type pos_ui_extension to the ui_extension migration process


cc @isaacroldan

js-goupil · 2024-07-05T17:21:21Z

packages/app/src/cli/services/context/prompts.ts

@@ -41,13 +41,15 @@ export async function extensionMigrationPrompt(
    .map((name) => `"${name}"`)
    .join(', ')

+  const migrationEndType = toMigrate.map(({local}) => `"${local.type}"`).join(', ')


Followed the same structure as the other variables in the prompt, but oddly enough this always ends up being one value in the toMigrate, even if there are two extensions to be migrated. Then if you deploy again, it will go to the second extension that needs to be migrated.

@isaacroldan any ideas?

Do you mean you have 2 local ui_extensions and 2 remote pos_ui_extension but only 1 in the toMigrate?

The code at getUIExtensionsToMigrate is pretty simple, it just finds matching extensions locally and remotely, you can debug that method and see why it might not match a specific extension.

Also, specifically about this line of code, you are not filtering for duplicated types here, check how uniqueMigrationTypes does it just above.

js-goupil · 2024-07-05T17:21:29Z

packages/app/src/cli/services/context/prompts.ts

  renderInfo({
    headline: "Extension migrations can't be undone.",
    body: `Your ${migrationNames} configuration has been updated. Migrating gives you access to new features and won't impact the end user experience. All previous extension versions will reflect this change.`,
  })

  const confirmMessage = includeRemoteType
-    ? `Yes, confirm migration from ${uniqueMigrationTypes}`
+    ? `Yes, confirm migration from ${uniqueMigrationTypes} to ${migrationEndType}`


This comment has been minimized.

Sign in to view

js-goupil requested review from byrichardpowell, rtymchyk, vividviolet and vctrchu July 4, 2024 16:26

rtymchyk approved these changes Jul 4, 2024

View reviewed changes

vividviolet reviewed Jul 4, 2024

View reviewed changes

js-goupil added 2 commits July 4, 2024 17:42

Added pos_ui_extension to migration for ui_extension

3ed3400

Removed console log

c09a2fc

js-goupil force-pushed the js-migration-pos-to-ui branch from aea2008 to c09a2fc Compare July 4, 2024 17:42

Added test case for POS

f973cb6

js-goupil force-pushed the js-migration-pos-to-ui branch from f973cb6 to aebbe10 Compare July 4, 2024 18:03

js-goupil requested a review from vividviolet July 4, 2024 18:03

vividviolet approved these changes Jul 4, 2024

View reviewed changes

vividviolet reviewed Jul 5, 2024

View reviewed changes

Updated prompt for migration and added changeset

cb06f10

js-goupil force-pushed the js-migration-pos-to-ui branch from aebbe10 to cb06f10 Compare July 5, 2024 17:19

js-goupil commented Jul 5, 2024

View reviewed changes

js-goupil added this pull request to the merge queue Jul 5, 2024

Merged via the queue into main with commit 8a42356 Jul 5, 2024

js-goupil deleted the js-migration-pos-to-ui branch July 5, 2024 18:25

shopify-shipit bot temporarily deployed to nightly July 6, 2024 10:34 Inactive

shopify-shipit bot had a problem deploying to production July 10, 2024 15:14 Failure

shopify-shipit bot temporarily deployed to production July 10, 2024 15:20 Inactive

shopify-shipit bot had a problem deploying to stable_3_64 July 17, 2024 07:28 Failure

shopify-shipit bot had a problem deploying to stable_3_64 July 17, 2024 07:30 Failure

shopify-shipit bot temporarily deployed to stable_3_64 July 17, 2024 07:31 Inactive

its-a-lisa mentioned this pull request Aug 1, 2024

[Snyk] Upgrade @shopify/cli from 3.45.0 to 3.64.0 its-a-lisa/plasmic#670

Open

WontonSam mentioned this pull request Aug 4, 2024

[Snyk] Upgrade @shopify/cli from 3.41.1 to 3.64.0 WontonSam/function-examples#39

Open

snyk-io bot mentioned this pull request Aug 14, 2024

[Snyk] Upgrade @shopify/cli from 3.41.1 to 3.64.1 WontonSam/function-examples#41

Open

WontonSam mentioned this pull request Aug 18, 2024

[Snyk] Upgrade @shopify/cli from 3.41.1 to 3.64.1 WontonSam/function-examples#43

Open

snyk-io bot mentioned this pull request Aug 24, 2024

[Snyk] Upgrade @shopify/cli from 3.41.1 to 3.65.0 WontonSam/function-examples#45

Open

NathanJolly mentioned this pull request Sep 6, 2024

Njo/add migration note Shopify/ui-extensions#2328

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pos_ui_extension to migration for ui_extension #4162

Add pos_ui_extension to migration for ui_extension #4162

js-goupil commented Jul 4, 2024 •

edited

Loading

This comment has been minimized.

rtymchyk left a comment •

edited

Loading

vividviolet left a comment

github-actions bot commented Jul 4, 2024 •

edited

Loading

js-goupil commented Jul 4, 2024

vividviolet left a comment

js-goupil commented Jul 4, 2024 •

edited

Loading

vividviolet commented Jul 4, 2024

isaacroldan commented Jul 5, 2024

vividviolet commented Jul 5, 2024

isaacroldan commented Jul 5, 2024

github-actions bot commented Jul 5, 2024

js-goupil commented Jul 5, 2024

vividviolet left a comment •

edited

Loading

isaacroldan commented Jul 5, 2024

js-goupil Jul 5, 2024

js-goupil Jul 5, 2024

vividviolet Jul 5, 2024

isaacroldan Jul 8, 2024

js-goupil Jul 5, 2024

Add pos_ui_extension to migration for ui_extension #4162

Add pos_ui_extension to migration for ui_extension #4162

Conversation

js-goupil commented Jul 4, 2024 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Post-release steps

Measuring impact

Checklist

This comment has been minimized.

rtymchyk left a comment • edited Loading

Choose a reason for hiding this comment

vividviolet left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 4, 2024 • edited Loading

Coverage report

Test suite run success

js-goupil commented Jul 4, 2024

vividviolet left a comment

Choose a reason for hiding this comment

js-goupil commented Jul 4, 2024 • edited Loading

vividviolet commented Jul 4, 2024

isaacroldan commented Jul 5, 2024

vividviolet commented Jul 5, 2024

isaacroldan commented Jul 5, 2024

github-actions bot commented Jul 5, 2024

js-goupil commented Jul 5, 2024

vividviolet left a comment • edited Loading

Choose a reason for hiding this comment

isaacroldan commented Jul 5, 2024

js-goupil Jul 5, 2024

Choose a reason for hiding this comment

js-goupil Jul 5, 2024

Choose a reason for hiding this comment

vividviolet Jul 5, 2024

Choose a reason for hiding this comment

isaacroldan Jul 8, 2024

Choose a reason for hiding this comment

js-goupil Jul 5, 2024

Choose a reason for hiding this comment

js-goupil commented Jul 4, 2024 •

edited

Loading

rtymchyk left a comment •

edited

Loading

github-actions bot commented Jul 4, 2024 •

edited

Loading

js-goupil commented Jul 4, 2024 •

edited

Loading

vividviolet left a comment •

edited

Loading