Add flag to allow —unpublished to reuse theme names

[jeffreyguenther]

[!NOTE] I have opened this PR early to allow for conversations on the flag name.

Also, it's my style to open PRs as soon as the first line is written so the work can be done the open.

Will remove this line once the PR is set for review.

WHY are these changes introduced?

Fixes #2699

shopify theme push —theme <name> --unpublished

Repeated calls the above command generates multiple themes with identical names. If you're using theme names to map to git branches in CI, this causes a problem. You need an idempotent way to push themes.

We want something like:

shopify theme push --theme <name> —-unpublished --upsert

We want to retain the ability to push multiple themes with the same name as that mirrors the Online Store UI behaviour. Therefore, we add a flag to modify how --unpublished works in a similar manner to how --unpublished modifies how a basic push works.

WHAT is this pull request doing?

In this PR, I add a flag to optionally allow to --unpublished to update a theme if there is already one with the passed name. The flag is dependent on --unpublished so we should get the nice OCLIF errors if it's passed without --unpublished

Here's my rationale:

This command pushes to a theme named "Iteration", but the theme with that name must exist. If it doesn't, an error is output.

shopify theme push --theme "Iteration"

So we add the --unpublished flag, to allow us to create a new, unpublished theme with the passed name.

shopify theme push --theme "Iteration" --unpublished

Then, we want to provide more information to this push to reuse the theme with the matching name if it exists.

shopify theme push --theme "Iteration" --unpublished --upsert

How to test your changes?

Setup the project locally and run the following multiple times on a test store.

pnpm shopify theme push --theme fancy-branch-name --unpublished --upsert

You should only see a single instance of a theme named fancy-branch-name

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

[github-actions[bot]]

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. → If there's no activity within a week, then a bot will automatically close this. Thanks for helping to improve Shopify's dev tooling and experience.

[jeffreyguenther]

Still working.

[github-actions[bot]]

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. → If there's no activity within a week, then a bot will automatically close this. Thanks for helping to improve Shopify's dev tooling and experience.

[jeffreyguenther]

Been delayed by client work, but this will finished one day.

[github-actions[bot]]

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. → If there's no activity within a week, then a bot will automatically close this. Thanks for helping to improve Shopify's dev tooling and experience.

[jeffreyguenther]

Returning shortly.

[jeffreyguenther]

@jamesmengo for your consideration.

[jeffreyguenther]

@mxstbr, @karreiro or @graygilmore

[jamesmengo]

Hey @jeffreyguenther! Sorry for the late response - I'm switching teams internally so will no longer be working in this area, but I'll raise this directly to the team to make sure this gets eyes.

Thanks for contributing and for all the work you do for our tooling!

[aswamy]

Hey @jeffreyguenther thanks for PR. If you wanted to push to the same theme every time, wouldn't you just want to use the theme's id instead?

shopify theme push --theme <id>

The Con of this approach is obviously it wouldn't work on the first call. i.e. you'd have to make the first theme manually, then the subsequent calls would work.

I have a few concerns with the approach in this PR, but hoping to get some light on it:

• --unpublished by design is known to create a theme so not sure if reusing this flag is the best idea
• if you do shopify theme push --theme "production" --unpublished --upsert and the theme was a production theme, would it "unpublish" the production theme?
  • Or we just say shopify theme push --theme "production" --upsert will always update the theme if it exists, OR create an unpublished theme by default (no --unpublished needed 🤷)

[jeffreyguenther]

My use case for this feature is in CI to generate preview themes. Ideally, you can run an idempotent create command in the GitHub workflow so that the workflow doesn't need to care whether the theme exists. This is why I went with an "upsert" approach.

I kept --unpublished in because it helps communicate that the push results in a new unpublished theme. I think it can be removed if we can communicate the semantics a different way.

With regard to pushing to a live theme, I think the only way to push to the live theme should be with the flag --allow-live. shopify theme push --theme "production" --unpublished --upsert should return an error saying that the theme you've named is the live theme.

I think I may need to collect the different potential combinations of the flags on push as there are some funky dependencies and implied precedence in the code. Once we can see how the flags interact, I think the solution will be more obvious.

[aswamy]

@jeffreyguenther I have some ideas on how we can possibly improve the current solution. Let me know what you think.

Instead of having another flag that's dependent on --unpublished flag, we could just make --unpublished a string arg instead of a boolean arg.

i.e.

• shopify theme push --theme "test theme" --unpublished will implicitly have "create" as a value for --unpublished
  • --unpublished "create": create a new unpublished theme no matter what
  • --unpublished: has "create" as default value (same as above)
  • --unpublished "upsert": creates a new unpublished theme if it doesn't exist
    • if a live theme exists with the same name, create a new one instead

This solution wouldn't break for existing CLI users since shopify theme push --theme "test theme" --unpublished is still a valid command and does the same operation as previous versions.

[jeffreyguenther]

I like this approach. Yesterday, I went through the possible ways --unpublished can be used and was leaning towards something like this with a new flag like --upsert.

I think switching the flag from a boolean to a string is the likely the cleanest way to do this. It lets us configure --unpublished's behaviour.

--unpublished "upsert": creates a new unpublished theme if it doesn't exist if a live theme exists with the same name, create a new one instead

What do you think about throwing an error in this case? I think creating the additional theme with the same name breaks the upsert semantics. I would expect that we either create a new theme because it doesn't exist, or we'd update the existing one.

[aswamy]

What do you think about throwing an error in this case? I think creating the additional theme with the same name breaks the upsert semantics. I would expect that we either create a new theme because it doesn't exist, or we'd update the existing one.

Since this is explicitly saying to upsert an unpublished theme, i dont think it breaks it. I'd read the following command as, "if you dont have an unpublished theme with this name, create one":

shopify theme push --theme "my theme" --unpublished "upsert"

However, I WOULD throw an error if you pass --unpublished "upsert" when you pass a theme ID instead of theme name since the behaviour of --theme <id> is always a "update" i.e.

shopify theme push --theme <id> --unpublished "upsert"

[jeffreyguenther]

Makes sense to me. Thanks for collaboration! I'll get to work on updating the PR.

[jeffreyguenther]

@aswamy Ready for review. I think I've followed Shopify's style. Standing by for feedback!

In the meantime, I'll do some manual testing.

[graygilmore]

I'm not sure if running /snapit will work on a forked repo PR but it'll make testing a little bit easier for folks so I'll try!

[graygilmore]

/snapit

Error: `/snapit` is not supported on pull requests from forked repositories.

[jeffreyguenther]

Is there any interest in me finishing this work?

[github-actions[bot]]

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. → If there's no activity within a week, then a bot will automatically close this. Thanks for helping to improve Shopify's dev tooling and experience.

[jeffreyguenther]

Will return to this shortly.

[github-actions[bot]]

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. → If there's no activity within a week, then a bot will automatically close this. Thanks for helping to improve Shopify's dev tooling and experience.

[jeffreyguenther]

yup, still matters

[github-actions[bot]]

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. → If there's no activity within a week, then a bot will automatically close this. Thanks for helping to improve Shopify's dev tooling and experience.