Shopify / themekit

Shopify theme development command line tool.
https://shopify.dev/tools/theme-kit
MIT License
1.19k stars 374 forks source link

Update docs: theme new deploys current working directory & strips comments #910

Open th0rgall opened 3 years ago

th0rgall commented 3 years ago

Describe the bug

The documentation of theme new does not fully describe what the command will do. This can result in unexpected code to be uploaded, and the stripping of config.yml comments.

Update: I noted that this is related to a very recent feature, so I guess that's why it's not documented yet. See linked issue below.

To Reproduce Steps to reproduce the behavior:

  1. run command 'theme new --env=my-environment --name=theme'
  2. run command 'theme deploy --env=my-environtment" (with a non-default theme in the local working directory)

The documentation (https://shopify.dev/tools/theme-kit/command-reference#new) specifies this:

Creates a new theme on the specified Shopify store, initializes your configuration using your API password and new theme ID, and generates and uploads default templates to make your theme valid.

But "uploads default templates" is not what is happening. The non-default current working directory theme is already being uploaded.

Heres a CLI log:

➜  moomin-shopify-theme git:(feature/germany-preview) ✗ theme new -e=test_de --name="omitted"
[omitted.myshopify.com] theme created
[omitted.myshopify.com] config created
Exists assets.
        Created assets/application.css.liquid.
        Created assets/application.js.
Exists config.
        Exists config/settings_data.json.
        Exists config/settings_schema.json.
Exists layout.
        Exists layout/theme.liquid.
Exists locales.
        Exists locales/en.default.json.
Exists templates.
        Exists templates/page.contact.liquid.
        Exists templates/page.liquid.
        Exists templates/product.liquid.
        Exists templates/article.liquid.
        Exists templates/blog.liquid.
        Exists templates/cart.liquid.
        Exists templates/index.liquid.
        Exists templates/list-collections.liquid.
        Exists templates/search.liquid.
        Exists templates/404.liquid.
        Exists templates/collection.liquid.
        Created templates/collection.list.liquid.
        Exists templates/gift_card.liquid.
Exists templates/customers.
        Exists templates/customers/register.liquid.
        Exists templates/customers/reset_password.liquid.
        Exists templates/customers/account.liquid.
        Exists templates/customers/activate_account.liquid.
        Exists templates/customers/addresses.liquid.
        Exists templates/customers/login.liquid.
        Exists templates/customers/order.liquid.
[omitted.com] uploading new files to shopify
[test_de] 318|318 [==============================================================================]  100 %
[test_de] 318 files, Updated: 318
➜  moomin-shopify-theme git:(feature/germany-preview) ✗ theme deploy --env=test_de
[test_de] 318|318 [==============================================================================]  100 %
[test_de] 318 files, No Change: 318

Note the uploading of the current working directory theme at the end of the new command

[omitted.com] uploading new files to shopify
[test_de] 318|318 [==============================================================================]  100 %
[test_de] 318 files, Updated: 318

Also, note the 318 files, No Change: 318 of the deploy command that shows that the local theme was already uploaded. I suspect this line https://github.com/Shopify/themekit/blob/b204edd3a4e1d96559bf2bc75272e0f28e4a07cc/cmd/new.go#L56 is responsible for the deploy.

A related unexpected change

The 1-sentence doc further specifies:

initializes your configuration using your API password and new theme ID

When running theme new with an --env switch, what actually happens is this:

  1. The current config.yml is read
  2. All the comments & whitespace are stripped.
  3. The newly created theme ID is filled into the env that was given.
  4. This processed config.yml is written again

I think that's good behavior, but it's not documented. I didn't know my comments were going to be stripped.

Expected behavior

Environment (please complete the following information):