Skip to content

theme pull/push: Pass development theme from CLI 3’s to CLI 2’s local storage#1410

Merged
Poitrin merged 5 commits intomainfrom
fix-1387
Mar 3, 2023
Merged

theme pull/push: Pass development theme from CLI 3’s to CLI 2’s local storage#1410
Poitrin merged 5 commits intomainfrom
fix-1387

Conversation

@Poitrin
Copy link
Copy Markdown
Contributor

@Poitrin Poitrin commented Feb 20, 2023

WHY are these changes introduced?

Fixes #1387

WHAT is this pull request doing?

  • Retrieves, when shopify theme pull or … push is called, development theme ID, if existing, and passes it from CLI 3 to 2, so that CLI 2 can set the development theme ID in its own local storage (ShopifyCLI::DB). That way, development theme is not treated as foreign development and therefore not hidden from the select list anymore.
  • Displays [yours] label in shopify theme open list, if applicable.

How to test your changes?

Note: (pnpm )shopify:run → CLI 3, shopify-dev → CLI 2

theme pull

  1. pnpm build
  2. Verify that shopify:run theme list’s and shopify-dev theme list’s [yours] development themes have different IDs, if development themes exist.
  3. shopify:run theme init, with name e.g. draft-1
  4. shopify:run theme dev --path="./draft-1"
  5. Note down the ID, and wait until command is finished
  6. shopify:run theme pull --path="./draft-2"
  7. Verify that the pull command lists a development theme with [yours] label. Note down the name and Ctrl+C.
  8. Verify that shopify:run theme list and shopify-dev theme list both list the [yours] development theme, with the ID and name you noted down before.

theme push

  1. shopify-dev theme delete <Theme ID of the development theme you just created>
  2. Verify that shopify-dev theme list does not list the theme anymore
  3. shopify:run theme dev --path="./draft-1" and note down the ID of the new development theme
  4. shopify:run theme list → Note down the name of the development theme
  5. shopify:run theme push --path="./draft-1" and verify that the development theme with the name you noted down is listed

theme open

  1. shopify:run theme open
  2. Verify that the development theme you created is tagged with [development] [yours].

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

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

This comment has been minimized.

@Poitrin Poitrin force-pushed the fix-1387 branch 2 times, most recently from c07d411 to d68918c Compare February 20, 2023 15:57
@Poitrin Poitrin marked this pull request as draft February 20, 2023 16:04
@Poitrin Poitrin changed the title theme pull: Pass development theme from CLI 3’s to CLI 2’s local storage theme pull/push: Pass development theme from CLI 3’s to CLI 2’s local storage Feb 20, 2023
@Poitrin Poitrin force-pushed the fix-1387 branch 4 times, most recently from ebfe37e to df61baf Compare February 21, 2023 08:42
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 21, 2023

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 72.67% 4159/5723
🟡 Branches 69.88% 1875/2683
🟡 Functions 70.13% 1101/1570
🟡 Lines 73.97% 3970/5367

Test suite run success

1074 tests passing in 535 suites.

Report generated by 🧪jest coverage report action from 3185b2a

@Poitrin Poitrin marked this pull request as ready for review February 21, 2023 09:02
@Poitrin Poitrin requested a review from a team February 21, 2023 09:02
Copy link
Copy Markdown
Contributor

@mgmanzella mgmanzella left a comment

Choose a reason for hiding this comment

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

noice!! had a couple questions

Comment thread packages/theme/src/cli/commands/theme/pull.ts
Comment thread packages/theme/src/cli/services/open.ts
@Poitrin Poitrin requested review from a team, Arkham and alvaro-shopify and removed request for a team March 2, 2023 15:17
@Poitrin
Copy link
Copy Markdown
Contributor Author

Poitrin commented Mar 2, 2023

Can someone from @Shopify/cli-foundations 🎩 this PR, to confirm that it's only not working on @mgmanzella’s machine? Thank you 😊

Update: PR was able to get 🎩ted :)

Copy link
Copy Markdown
Contributor

@mgmanzella mgmanzella left a comment

Choose a reason for hiding this comment

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

nice job!! tophatting looked great, thank you @Poitrin!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 3, 2023

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

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/node/themes/theme-manager.d.ts
@@ -8,6 +8,6 @@ export declare abstract class ThemeManager {
     protected abstract context: string;
     constructor(adminSession: AdminSession);
     findOrCreate(): Promise<Theme>;
-    protected fetch(): Promise<Theme | undefined>;
+    fetch(): Promise<Theme | undefined>;
     private create;
 }
\ No newline at end of file

@Poitrin Poitrin merged commit 067199c into main Mar 3, 2023
@Poitrin Poitrin deleted the fix-1387 branch March 3, 2023 08:59
Poitrin pushed a commit to Shopify/shopify-cli that referenced this pull request Mar 3, 2023
### WHY are these changes introduced?
Backports changes from CLI 3 to CLI 2, until CLI 2 is sunset.

### WHAT is this pull request doing?

Back-port from
* Shopify/cli#1369
* Shopify/cli#1410
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.

theme pull (CLI 2) does not show development theme (created by CLI 3) in the select list

2 participants