[Themes][Bugfix] - Fix unpublished themes being marked as development themes #3798

jamesmengo · 2024-04-26T17:31:08Z

WHY are these changes introduced?

[Bug]: shopify theme dev is losing track of the development theme #3715

I left some explanations about why this is happening in the diff view

WHAT is this pull request doing?

1) Prevents unpublished themes from being marked as `[unpublished][yours]` when creating a theme via `shopify theme push -u`

Command	Before	After
`-d`	✅ `[development][yours]`	✅ `[development][yours]`
`-u`	🐛 `[unpublished][yours]`	✅ `[unpublished]`
`-u -d`	🐛`[unpublished][yours]`	✅ `[unpublished]`

2) Flushes the local storage for development themes

This will reset all development themes (which are ephemeral anyway), and can still be accessed

How to test your changes?

Change 1

pnpm shopify theme push -u --path=<PATH>
pnpm shopify theme list should show your new theme with [unpublished]
You can also try pnpm shopify theme push -u -d --path=<PATH>

Change 2

Create a development theme (either corrupt or working state)
Run pnpm shopify theme list
[yours] should be empty

BEFORE

AFTER

Post-release steps

Maybe this isn't the only problem - need to monitor to see if this resolves the issues in dev
You may need to run shopify theme delete and delete the existing [unpublished][yours] theme if it exists
[Theme] Add error handling when development theme role gets mismatched #3802

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 · 2024-04-26T17:31:28Z

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/app-management

jamesmengo · 2024-04-26T17:34:15Z

packages/theme/src/cli/commands/theme/push.ts

@@ -149,22 +151,24 @@ export default class Push extends ThemeCommand {
      return
    }

-    const developmentThemeManager = new DevelopmentThemeManager(adminSession)


DevelopmentThemeManager marks all themes as development - any theme created via the push command will be marked as [yours].

To address this, I've created a ThemeManager and renamed the base class to BaseThemeManager

Take the code tour

{ "$schema": "https://aka.ms/codetour-schema", "title": "Unpublished Themes", "steps": [ { "file": "src/cli/commands/theme/push.ts", "description": "Here, we always use the developmentThemeManager when the `-u` flag is passed\n\nNote that the -u flag is required to create a new theme (-d and -t) specify filters for existing themes\n\nSide side note - this is an indication of some brittle behaviour, but `-t` actually specifies the name of the theme when the `-u` flag is provided. Pretty weird imo. Since the goal is parity first, I won't change any of this for the moment.", "line": 160 }, { "file": "../cli-kit/src/public/node/themes/theme-manager.ts", "description": "We were using the create method to create a UNPUBLISHED theme, but this callback was getting triggered in DevelopmentThemeManager", "line": 48 }, { "file": "src/cli/utilities/development-theme-manager.ts", "description": "this is the reason why all themes created via the push command are marked as the development theme", "line": 29 } ], "ref": "79854e6cc3e591d252a90e2b2071e80299025adc" }

github-actions · 2024-04-26T17:36:05Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	71.79%	6942/9670
🟡	Branches	68.71%	3419/4976
🟡	Functions	71.22%	1858/2609
🟡	Lines	73.09%	6543/8952

Test suite run success

1633 tests passing in 763 suites.

Report generated by 🧪jest coverage report action from 3ecdbcd

packages/cli-kit/src/public/node/themes/base-theme-manager.ts

github-actions · 2024-04-26T19:23:06Z

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/base-theme-manager.d.ts

import { AdminSession } from '@shopify/cli-kit/node/session';
import { Theme } from '@shopify/cli-kit/node/themes/types';
import { Role } from '@shopify/cli-kit/node/themes/utils';
export declare abstract class BaseThemeManager {
    protected adminSession: AdminSession;
    protected themeId: string | undefined;
    protected abstract setTheme(themeId: string): void;
    protected abstract removeTheme(): void;
    protected abstract context: string;
    constructor(adminSession: AdminSession);
    findOrCreate(): Promise<Theme>;
    fetch(): Promise<Theme | undefined>;
    create(themeRole?: Role, themeName?: string): Promise<Theme>;
}

Existing type declarations

We found no diffs with existing type declarations

packages/theme/src/cli/utilities/unpublished-theme-manager.ts

jamesmengo · 2024-04-26T19:44:40Z

packages/theme/src/cli/utilities/unpublished-theme-manager.ts

+
+  protected setTheme(themeId: string): void {
+    this.themeId = themeId
+  }


Should I be storing this or doing anything with this?

Currently this class just allows us to call create WITHOUT setting the development theme, so there's no immediate use case that's popping out at me

I could also no-op this

The CLI doesn't need to store references to unpublished themes; it only saves a reference to the development theme and another to the host theme.

I wonder if the UnpublishedThemeManager abstraction is the best one to support the push command because it maintains state, leading readers to think that the state matters for unpublished themes. ThemeManager does provide some convenience for creating themes, but I believe a better path would involve extracting those features from ThemeManager.

jamesmengo · 2024-04-26T19:45:48Z

packages/theme/src/cli/commands/theme/push.ts

    if (!flags.stable) {
-      const {live, development, unpublished, path, nodelete, theme, publish, json, force, ignore, only} = flags


I've moved this logic into createOrSelectTheme below

jamesmengo · 2024-04-26T19:56:15Z

packages/theme/src/cli/commands/theme/push.ts

+): Promise<Theme | undefined> {
+  const {live, development, unpublished, theme} = flags
+
+  if (development) {


Decision / Diverging from Ruby

When multiple flags are specified for the theme role, create a single theme with the least risky / invasive role assigned to it

Development -> Unpublished -> Live

Rationale:

The logic in the Ruby implementation is pretty smelly.

shopify theme push -u -d --stable creates two themes, but was only pushing the files to the unpublished theme

[unpublished]

[development][yours] <- this theme is empty

There are some more weird issues here that I would at least like to understand / document even if we don't end up changing things

[Theme][Enhancement] Review ergonomics of theme selection flags in the push command #3801

Question: Do we even want to allow for creation of a development theme with theme push? @karreiro

The --stable flag creates a new development theme when there is none that already exist but it looks like that's coming from the TS wrapper. May be off base here

I don't think it's related to the Ruby implementation, as we may notice here. The Ruby implementation adopts similar reasoning with the theme ID taking precedence.

The creation of the extra theme appears to be related to the development theme creation extracted to TypeScript via the --development-theme-id flag. Thus, this seems to be more like a problem in the TS <-> Ruby interoperability than on how each implementation selects the theme.

Question: Do we even want to allow for creation of a development theme with theme push? @karreiro

Yes, keeping backward compatibility in mind, it's a good idea to create a development theme.

lukeh-shopify

Thanks for the detailed information. It's worth getting a second opinion as I'm not as familiar with the push logic, but overall it seems to make sense 😁

lukeh-shopify · 2024-04-29T19:13:30Z

packages/theme/src/cli/commands/theme/push.ts

+    const themeManager = new UnpublishedThemeManager(adminSession)
+    return themeManager.create(UNPUBLISHED_THEME_ROLE, themeName)
+  } else {
+    if (live && !flags['allow-live'] && !(await confirmPushToLiveTheme(adminSession.storeFqdn))) {


previously we had to check selected_theme.role === 'live', are we safe to only rely on the flag being provided? does live get auto populated if not provided?

thanks for raising this!
I'm going to move this check to us the role again - this will cover cases where the live theme ID is provided via the theme flag.
Also going to add a test for this!

packages/theme/src/cli/commands/theme/push.ts

packages/theme/src/cli/utilities/unpublished-theme-manager.test.ts

lukeh-shopify · 2024-04-29T19:18:57Z

packages/theme/src/cli/commands/theme/push.test.ts

Should these also have a test for the [yours] logic, or is it outside of the scope of this class?

imo, that falls outside the scope of this class since [yours] is detected by the theme selector

Ideally, these tests should only know that a development theme is created, but I'm going to add assertions on which ThemeManager is used to provide some extra clarity (don't love this, but I think this balances effort/reward)

Maybe time for some integration tests? wdyt?

…live flag

jamesmengo · 2024-04-30T01:09:44Z

packages/theme/src/cli/services/local-storage.ts

@@ -24,7 +24,7 @@ function themeLocalStorage() {
 function developmentThemeLocalStorage() {
  if (!_developmentThemeLocalStorageInstance) {
    _developmentThemeLocalStorageInstance = new LocalStorage<DevelopmentThemeLocalStorageSchema>({
-      projectName: 'shopify-cli-development-theme-conf',
+      projectName: 'shopify-cli-development-theme-config',


flushes local storage key

karreiro

Thank you, @jamesmengo! I've left only some minor comments not blocking to merge this PR. Great stuff!

karreiro · 2024-04-30T08:47:26Z

packages/theme/src/cli/commands/theme/push.test.ts

+  describe('createOrSelectTheme', async () => {
+    beforeEach(() => {
+      vi.mocked(DevelopmentThemeManager.prototype.findOrCreate).mockResolvedValue(
+        buildTheme({id: 1, name: 'Theme', role: DEVELOPMENT_THEME_ROLE})!,
+      )
+      vi.mocked(UnpublishedThemeManager.prototype.create).mockResolvedValue(
+        buildTheme({id: 2, name: 'Unpublished Theme', role: UNPUBLISHED_THEME_ROLE})!,
+      )
+      vi.mocked(findOrSelectTheme).mockImplementation(
+        async (_session: AdminSession, options: {header?: string; filter: FilterProps}) => {
+          if (options.filter.live) {
+            return buildTheme({id: 3, name: 'Live Theme', role: LIVE_THEME_ROLE})!
+          } else if (options.filter.theme) {
+            return buildTheme({id: 4, name: options.filter.theme, role: DEVELOPMENT_THEME_ROLE})!
+          } else {
+            return buildTheme({id: 5, name: 'Theme', role: DEVELOPMENT_THEME_ROLE})!
+          }
+        },
+      )
+    })


(optional) Personally, I'd move these mocks to the // Given sections. Alternatively, we could mock at the local-storage.ts/themes/api.ts level.

karreiro · 2024-04-30T08:56:47Z

packages/theme/src/cli/utilities/unpublished-theme-manager.ts

+
+  protected setTheme(themeId: string): void {
+    this.themeId = themeId
+  }


The CLI doesn't need to store references to unpublished themes; it only saves a reference to the development theme and another to the host theme.

I wonder if the UnpublishedThemeManager abstraction is the best one to support the push command because it maintains state, leading readers to think that the state matters for unpublished themes. ThemeManager does provide some convenience for creating themes, but I believe a better path would involve extracting those features from ThemeManager.

karreiro · 2024-04-30T09:12:58Z

packages/theme/src/cli/commands/theme/push.ts

+): Promise<Theme | undefined> {
+  const {live, development, unpublished, theme} = flags
+
+  if (development) {


I don't think it's related to the Ruby implementation, as we may notice here. The Ruby implementation adopts similar reasoning with the theme ID taking precedence.

The creation of the extra theme appears to be related to the development theme creation extracted to TypeScript via the --development-theme-id flag. Thus, this seems to be more like a problem in the TS <-> Ruby interoperability than on how each implementation selects the theme.

Question: Do we even want to allow for creation of a development theme with theme push? @karreiro

Yes, keeping backward compatibility in mind, it's a good idea to create a development theme.

karreiro · 2024-04-30T09:29:59Z

(proceeding with the merge, so partners can benefit from this fix asap)

jamesmengo force-pushed the jmeng/devthemes branch from 800657b to a99b2f8 Compare April 26, 2024 17:31