[CLI UI kit - Themes adoption] Extract the ownership of development themes by Poitrin · Pull Request #1169 · Shopify/cli

Poitrin · 2023-01-20T12:10:33Z

WHY are these changes introduced?

Fixes https://github.com/Shopify/internal-cli-foundations/issues/509
Fixes #1084
Fixes #1263

Requires Shopify/shopify-cli#2724

WHAT is this pull request doing?

Generates Development Theme ID and calls Themes API in Node.js CLI.
Depending on the shopify theme command: passes ID of generated theme to Ruby CLI for further processing.

How to test your changes?

Make sure that your local Ruby CLI uses changes from Shopify/shopify-cli#2724

Verify that (pnpm run )shopify theme info says Development Theme ID Not set
Create a new development theme: shopify theme dev --path="./my-theme"
Verify that shopify theme info now displays the ID for the created dev theme
Verify that shopify theme open -d opens the dev theme (with the correct ID)
Verify that shopify theme delete -d deletes the dev theme (after you confirm the deletion)
Verify that shopify theme info says Development Theme ID Not set again
Verify that shopify theme pull -d --path="./my-theme" throws the error No development theme ID has been set. Please create a development theme first.
Verify that shopify theme push -d --path="./my-theme" creates a new development theme.
Verify that shopify theme info now displays the ID for the created dev theme
Verify that shopify theme dev --path="./my-theme" uses the correct ID
Delete the theme with CLI 2.x (so that 3.x doesn't know anything about it): shopify(-dev) theme delete <your ID>
Verify that shopify theme dev --path="./my-theme" still works, but has created a new ID
Delete this theme also with CLI 2.x
Verify that shopify theme pull -d --path="./my-theme" throws the error Development theme <old ID> could not be found. Please create a new development theme.

`--theme-editor-sync`

shopify theme init a new theme.
Make sure that no development theme ID is set in CLI.
shopify theme dev --store="…" --path="./draft" --theme-editor-sync
Verify that CLI does not display any "Keep the remote version" prompt.

Keep Development Theme ID when switching stores

Execute shopify theme dev --path="./my-theme" --store=<store 1> and stop the server
Note down the theme ID that was created
Execute shopify theme dev --path="./my-theme" --store=<store 2> and stop the server
Note down the theme ID that was created
Execute shopify theme dev --path="./my-theme" --store=<store 1> again
Verify that the ID for store 1 was reused
Execute shopify theme dev --path="./my-theme" --store=<store 2> and stop the server
Verify that the ID for store 2 was reused

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.

github-actions · 2023-01-20T12:11:04Z

Thanks for your contribution!

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

Themes: @shopify/theme-developer-tools
UI extensions: @shopify/ui-extensions-cli
- Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
Hydrogen: @shopify/hydrogen
Other: @shopify/cli-foundations

github-actions · 2023-01-20T12:17:27Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	71.79% (-0.05% 🔻)	3936/5483
🟡	Branches	69.1% (-0.16% 🔻)	1771/2563
🟡	Functions	70.2% (+0.03% 🔼)	1034/1473
🟡	Lines	73.02% (-0.05% 🔻)	3754/5141

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / generate-theme-name.ts	100%	100%	100%	100%
🟢	`...` / replace-invalid-characters.ts	100%	100%	100%	100%
🔴	`...` / theme-manager.ts	0%	0%	0%	0%
🟢	`...` / theme-urls.ts	100%	100%	100%	100%
🟡	`...` / themes-api.ts	77.78%	73.68%	83.33%	79.41%
🔴	`...` / theme.ts	58.33%	40%	50%	44.44%
🟢	`...` / headers.ts	100%	90%	100%	100%
🔴	`...` / retry.ts	0%	100%	0%	0%
🟢	`...` / throttler.ts	79.31%	62.5%	84.62%	85.19%

Test suite run success

995 tests passing in 518 suites.

Report generated by 🧪jest coverage report action from 0ec8ec8

github-actions · 2023-01-20T12:17:31Z

Benchmark report

The following table contains a summary of the startup time for all commands.

Status	Command	Baseline (avg)	Current (avg)	Diff
🟢	`app build`	915.33 ms	919 ms	0.4 %
🟢	`app deploy`	1114 ms	1118.67 ms	0.42 %
🟢	`app dev`	1111 ms	1117.33 ms	0.57 %
🟢	`app env pull`	1011.33 ms	1021.67 ms	1.02 %
🟢	`app env show`	1009.33 ms	1020.33 ms	1.09 %
🟢	`app generate extension`	1075.67 ms	1082.33 ms	0.62 %
🟢	`app generate schema`	1036.67 ms	1052 ms	1.48 %
🟢	`app info`	1016.67 ms	1015.33 ms	-0.13 %
🟢	`app scaffold extension`	1081.33 ms	1081.67 ms	0.03 %
🟢	`app update-url`	984.33 ms	1000 ms	1.59 %
🟢	`theme check`	854 ms	865.67 ms	1.37 %
🟢	`theme delete`	969 ms	976.33 ms	0.76 %
🟢	`theme dev`	965.33 ms	972 ms	0.69 %
🟢	`theme help-old`	852.67 ms	862.33 ms	1.13 %
🟢	`theme info`	900 ms	902 ms	0.22 %
🟢	`theme init`	878.33 ms	879.33 ms	0.11 %
🟢	`theme language-server`	851 ms	856.67 ms	0.67 %
🟢	`theme list`	975 ms	982.33 ms	0.75 %
🟢	`theme open`	969.67 ms	986.33 ms	1.72 %
🟢	`theme package`	894.67 ms	907.67 ms	1.45 %
🟢	`theme publish`	969.67 ms	972 ms	0.24 %
🟢	`theme pull`	964 ms	976.67 ms	1.31 %
🟢	`theme push`	962.33 ms	973 ms	1.11 %
🟢	`theme serve`	965 ms	979 ms	1.45 %
🟢	`theme share`	962.67 ms	982 ms	2.01 %
🟢	`webhook trigger`	964.67 ms	966 ms	0.14 %

karreiro

Thank you for the updates, @Poitrin! 🚀

I've tried the PR and almost everything is working as expected. I've noticed a minor regression on this issue Shopify/shopify-cli#2433 / Shopify/shopify-cli#2463.

For fixing that scenario, I believe may rely on a hidden flag in the CL2 to differentiate themes created at runtime. Feel free to reach out if you need any extra context tho :)

Thanks again for this PR!

Poitrin · 2023-01-27T10:13:49Z

I've noticed a minor regression on this issue Shopify/shopify-cli#2433 / Shopify/shopify-cli#2463.

Phew, good to have you as a reviewer, to think of all these sneaky regressions 😥.

For fixing that scenario, I believe may rely on a hidden flag in the CL2 to differentiate themes created at runtime.

→ Shopify/shopify-cli#2724 + I updated this PR’s code and instructions

### WHY are these changes introduced? Part of https://github.com/Shopify/internal-cli-foundations/issues/509 Is required by Shopify/cli#1169 Issue #2433 was fixed with #2463. With Shopify/cli#1169, Node CLI needs to pass Ruby CLI a hidden flag to differentiate themes created at runtime. ### WHAT is this pull request doing? Introduces `--overwrite-json` flag that Node CLI can pass to Ruby CLI.

… themes more easily

karreiro

Thank you, @Poitrin! Very nice stuff with the persistent development theme IDs 🔥🚀

karreiro · 2023-02-01T08:54:06Z

+    flags = {
+      ...flags,
+      theme,
+      'overwrite-json': Boolean(flags['theme-editor-sync']),


When the development theme is not new, and (at the Ruby CLI level) developers select that they want to keep the remote version, the CLI shouldn't set the overwrite-json as true.

In other words, to cover that scenario, this line should be something like:

'overwrite-json': Boolean(flags['theme-editor-sync']) && theme.isCreatedAtRuntime(),

We may solve this minor issue in a separate PR tho #1263

It was in the end such a small change that I decided to include it in this PR.
Moreover, as you suggested to make my changes "ready for host theme changes", I moved parts of them into this PR.

…cal JSON files

Poitrin · 2023-02-03T09:51:23Z

        '**/public/node/plugins/tunnel.ts',
        '**/public/node/presets.ts',
        '**/public/node/result.ts',
+        '**/public/node/themes/**/*',


→ #1264 (comment)

matteodepalo

LGTM

isaacroldan reviewed Jan 20, 2023

View reviewed changes

Comment thread packages/cli-kit/src/store.ts Outdated

Poitrin force-pushed the fix-509 branch 10 times, most recently from 65d92fe to 752fb8a Compare January 23, 2023 14:21

Poitrin commented Jan 23, 2023

View reviewed changes

Comment thread packages/theme/src/cli/utilities/theme-selector/filter.test.ts Outdated

Comment thread packages/theme/src/cli/utilities/development-theme-manager.ts Outdated

Poitrin marked this pull request as ready for review January 23, 2023 14:24

This comment has been minimized.

Sign in to view

Poitrin requested a review from a team January 23, 2023 14:24

Extract the ownership of development themes

5a040b6

Poitrin force-pushed the fix-509 branch from 752fb8a to 5a040b6 Compare January 23, 2023 14:26

karreiro self-requested a review January 24, 2023 07:58

Julien Poitrin added 2 commits January 24, 2023 10:12

Merge branch 'main' into fix-509

4e6bb07

Changes after code review

b4b3db8

karreiro reviewed Jan 25, 2023

View reviewed changes

Julien Poitrin added 5 commits January 26, 2023 11:28

Merge branch 'main' into fix-509

77b6f6c

Resolve merge conflicts after latest changes on main

0cbd918

Pass --overwrite-json flag to Ruby CLI

ffd39cf

Merge branch 'main' into fix-509

39dc618

Update manifests

c6ee55b

Poitrin mentioned this pull request Jan 27, 2023

Introduce hidden --overwrite-json flag Shopify/shopify-cli#2724

Merged

4 tasks

Store one development theme ID per store

ebce87f

Julien Poitrin added 6 commits January 30, 2023 09:42

Merge branch 'main' into fix-509

c12c930

Move Development Theme ID Store into separate store, for storing host…

1a206a2

… themes more easily

Merge branch 'main' into fix-509

8a169a1

Update oclif.manifest.json

76804ce

Use getThemeStore

fd65276

Merge branch 'main' into fix-509

a428292

karreiro approved these changes Feb 1, 2023

View reviewed changes

Julien Poitrin added 3 commits February 3, 2023 08:49

Merge branch 'main' into fix-509

24faa1b

Do not let shopify theme dev --theme-editor-sync always override lo…

150749b

…cal JSON files

Move refactoring from fix-532 into fix-509

c1af075

Poitrin commented Feb 3, 2023

View reviewed changes

Poitrin requested a review from matteodepalo February 3, 2023 12:45

matteodepalo approved these changes Feb 6, 2023

View reviewed changes

Julien Poitrin added 2 commits February 6, 2023 10:17

Merge branch 'main' into fix-509

bf21e40

Refresh manifests

0ec8ec8

Poitrin merged commit 5ba869f into main Feb 6, 2023

Poitrin deleted the fix-509 branch February 6, 2023 09:42

gonzaloriestra mentioned this pull request Feb 8, 2023

[Bug]: --theme flag not working on shopify dev command #1309

Closed

2 tasks

jamesmengo mentioned this pull request Jul 16, 2024

[Themes][app dev][TAE] Find or Create host theme when preparing theme app extension preview #4156

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLI UI kit - Themes adoption] Extract the ownership of development themes#1169

[CLI UI kit - Themes adoption] Extract the ownership of development themes#1169
Poitrin merged 20 commits intomainfrom
fix-509

Poitrin commented Jan 20, 2023 •

edited

Loading

Uh oh!

github-actions Bot commented Jan 20, 2023

Uh oh!

Uh oh!

github-actions Bot commented Jan 20, 2023 •

edited

Loading

Uh oh!

github-actions Bot commented Jan 20, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

karreiro left a comment •

edited

Loading

Uh oh!

Poitrin commented Jan 27, 2023 •

edited

Loading

Uh oh!

karreiro left a comment •

edited

Loading

Uh oh!

karreiro Feb 1, 2023

Uh oh!

Poitrin Feb 3, 2023

Uh oh!

Poitrin Feb 3, 2023

Uh oh!

matteodepalo left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Poitrin commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

--theme-editor-sync

Keep Development Theme ID when switching stores

Measuring impact

Checklist

Uh oh!

github-actions Bot commented Jan 20, 2023

Uh oh!

Uh oh!

github-actions Bot commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Test suite run success

Uh oh!

github-actions Bot commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark report

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

karreiro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Poitrin commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karreiro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karreiro Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

Poitrin Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

Poitrin Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

matteodepalo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Poitrin commented Jan 20, 2023 •

edited

Loading

`--theme-editor-sync`

github-actions Bot commented Jan 20, 2023 •

edited

Loading

github-actions Bot commented Jan 20, 2023 •

edited

Loading

karreiro left a comment •

edited

Loading

Poitrin commented Jan 27, 2023 •

edited

Loading

karreiro left a comment •

edited

Loading