Skip to content

Extract the ownership of host themes to the CLI3#1264

Merged
Poitrin merged 7 commits intomainfrom
fix-532
Feb 15, 2023
Merged

Extract the ownership of host themes to the CLI3#1264
Poitrin merged 7 commits intomainfrom
fix-532

Conversation

@Poitrin
Copy link
Copy Markdown
Contributor

@Poitrin Poitrin commented Feb 1, 2023

WHY are these changes introduced?

Fixes https://github.com/Shopify/internal-cli-foundations/issues/532
Relates to https://github.com/Shopify/internal-cli-foundations/issues/509

WHAT is this pull request doing?

  • Lets CLI 3.x create the host theme for app extension and passes theme ID to CLI 2.x afterwards.
  • As packages app and themes both need theme creation and ID retrieval logic, moves whole logic into cli-kit.

How to test your changes?

  1. If you don't have an app yet: npm init @shopify/app@latest
  2. cd <your app directory>
  3. If you don't have an extension yet: npm run generate extension
  4. cd ../ – back into root dir
  5. Execute pnpm shopify app dev --path="<your app directory>" --store=<store 1>
  6. Click the link in Set up the TAE in the host theme and verify that the theme and app are loading
  7. Note down the ID of the host theme
  8. Verify that pnpm shopify theme list --store=<store 1> lists your host theme with roles [development] [yours]
  9. Execute pnpm shopify app dev --path="<your app directory>" --store=<store 2>
  10. Verify that a new theme with another ID has been created
  11. Verify that pnpm shopify theme list --store=<store 2> lists your host theme with roles [development] [yours]
  12. Execute pnpm shopify app dev --path="<your app directory>" --store=<store 1>
  13. Verify that the ID of the previously created host theme has been reused.

Measuring impact

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

  • Existing analytics will cater for this addition

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
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 1, 2023

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

@Poitrin Poitrin force-pushed the fix-532 branch 2 times, most recently from a15c93e to 55feff7 Compare February 1, 2023 16:11
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 1, 2023

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
73.47% (-0.24% 🔻)
4163/5666
🟡 Branches
70.48% (-0.12% 🔻)
1874/2659
🟡 Functions
71.43% (-0.33% 🔻)
1090/1526
🟡 Lines
74.8% (-0.26% 🔻)
3975/5314
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🔴
... / host-theme-manager.ts
0% 100% 0% 0%
🔴
... / conf.ts
0% 0% 0% 0%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / theme-extension-args.ts
92.31% (-7.69% 🔻)
50% 100%
92.31% (-7.69% 🔻)

Test suite run success

1049 tests passing in 535 suites.

Report generated by 🧪jest coverage report action from 8669158

@Poitrin Poitrin force-pushed the fix-532 branch 10 times, most recently from 8c98111 to 050f12a Compare February 2, 2023 14:59
@Poitrin Poitrin marked this pull request as ready for review February 2, 2023 15:04
Comment thread .eslintrc.cjs Outdated
Base automatically changed from fix-509 to main February 6, 2023 09:42
@karreiro karreiro self-requested a review February 13, 2023 08:58
Copy link
Copy Markdown
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Thank you, @Poitrin 🚀 Nice stuff, I've noticed only one minor issue.

Shopify CLI creates a host theme, because developers need to setup their theme app extension into a theme to preview it (it's the second step of their development workflow).

This PR really moves the creation of the host theme to a upper level, however we still need to pass a flag to the Ruby CLI mentioning that this theme is empty (or created at runtime).

Thus, the Ruby CLI may take care of populating the new theme with assets. Otherwise, the new development theme stays empty and developers cannot setup their theme app extension:

image

@Poitrin
Copy link
Copy Markdown
Contributor Author

Poitrin commented Feb 13, 2023

we still need to pass a flag to the Ruby CLI mentioning that this theme is empty (or created at runtime)

Excellent catch! Can you take a look again? :)

Copy link
Copy Markdown
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Thank you, @Poitrin! Great stuff 🔥🚀

@github-actions
Copy link
Copy Markdown
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

packages/cli-kit/dist/public/node/themes/conf.d.ts
import { LocalStorage } from '@shopify/cli-kit/node/local-storage';
import { AdminSession } from '@shopify/cli-kit/node/session';
type HostThemeId = string;
type StoreFqdn = AdminSession['storeFqdn'];
interface HostThemeLocalStorageSchema {
    [themeStore: StoreFqdn]: HostThemeId;
}
export declare function hostThemeLocalStorage(): LocalStorage<HostThemeLocalStorageSchema>;
export declare function getHostTheme(storeFqdn: StoreFqdn): string | undefined;
export declare function setHostTheme(storeFqdn: StoreFqdn, themeId: HostThemeId): void;
export declare function removeHostTheme(storeFqdn: StoreFqdn): void;
export {};

Existing type declarations

We found no diffs with existing type declarations

@github-actions
Copy link
Copy Markdown
Contributor

Benchmark report

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

Status Command Baseline (avg) Current (avg) Diff
🟢 app build 1119 ms 1121 ms 0.18 %
🟢 app deploy 1349.33 ms 1360.33 ms 0.82 %
🟢 app dev 1351.33 ms 1371 ms 1.46 %
🟢 app env pull 1243 ms 1239 ms -0.32 %
🟢 app env show 1231.67 ms 1238.67 ms 0.57 %
🟢 app function build 1098.33 ms 1111.67 ms 1.21 %
🟢 app function run 1102.33 ms 1106.33 ms 0.36 %
🟢 app function schema 1268.67 ms 1270.33 ms 0.13 %
🟢 app function typegen 1106 ms 1108.67 ms 0.24 %
🟢 app generate extension 1321.33 ms 1325.67 ms 0.33 %
🟢 app generate schema 1261.67 ms 1274.33 ms 1 %
🟢 app info 1230.67 ms 1245.67 ms 1.22 %
🟢 app scaffold extension 1318.33 ms 1320.67 ms 0.18 %
🟢 app update-url 1199.33 ms 1209.67 ms 0.86 %
🟢 theme check 1055.33 ms 1047.67 ms -0.73 %
🟢 theme console 1174.67 ms 1178.33 ms 0.31 %
🟢 theme delete 1182 ms 1188 ms 0.51 %
🟢 theme dev 1184.67 ms 1184 ms -0.06 %
🟢 theme help-old 1049.33 ms 1051.33 ms 0.19 %
🟢 theme info 1093.33 ms 1104.67 ms 1.04 %
🟢 theme init 1060.33 ms 1072 ms 1.1 %
🟢 theme language-server 1038.67 ms 1045 ms 0.61 %
🟢 theme list 1182.67 ms 1188 ms 0.45 %
🟢 theme open 1189.33 ms 1201 ms 0.98 %
🟢 theme package 1098.67 ms 1099.33 ms 0.06 %
🟢 theme publish 1174.67 ms 1197.33 ms 1.93 %
🟢 theme pull 1180.67 ms 1190 ms 0.79 %
🟢 theme push 1173 ms 1179.67 ms 0.57 %
🟢 theme serve 1183 ms 1189 ms 0.51 %
🟢 theme share 1164.67 ms 1174.33 ms 0.83 %
🟢 webhook trigger 1192.33 ms 1210 ms 1.48 %

@Poitrin Poitrin merged commit d2adeb5 into main Feb 15, 2023
@Poitrin Poitrin deleted the fix-532 branch February 15, 2023 09:47
@Poitrin Poitrin mentioned this pull request Feb 22, 2023
4 tasks
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