panoply / syncify

🤝 WIP ~ Shopify theme upload, download and watch development tool.
MIT License
132 stars 12 forks source link

Settings key injection into section schema causing API errors #38

Closed WolfGreyDev closed 2 months ago

WolfGreyDev commented 2 months ago

Problem

When using the @app type in a section schema, an additional settings key is injected into the final section/*.liquid file.

This will result in in a 422 (Unprocessable Entity) error when uploading the file to the store.

Example

sections/apps.liquid

<div class="{% if section.settings.include_margins %}page-width{% endif %}{% if settings.animations_reveal_on_scroll %} scroll-trigger animate--slide-in{% endif %}">
  {%- for block in section.blocks -%}
    {% render block %}
  {%- endfor -%}
</div>

{% schema %}
{
  "name": "t:sections.apps.name",
  "tag": "section",
  "class": "section",
  "settings": [
    {
      "type": "checkbox",
      "id": "include_margins",
      "default": true,
      "label": "t:sections.apps.settings.include_margins.label"
    }
  ],
  "blocks": [
    {
      "type": "@app"
    }
  ],
  "presets": [
    {
      "name": "t:sections.apps.presets.name"
    }
  ]
}
{% endschema %}

Expected result

The type block shouldn't have any settings key. See Shopify Docs.

sections/apps.liquid

<div class="{% if section.settings.include_margins %}page-width{% endif %}{% if settings.animations_reveal_on_scroll %} scroll-trigger animate--slide-in{% endif %}">
  {%- for block in section.blocks -%}
    {% render block %}
  {%- endfor -%}
</div>

{% schema %}
{"name":"t:sections.apps.name","tag":"section","class":"section","settings":[{"type":"checkbox","id":"include_margins","default":true,"label":"t:sections.apps.settings.include_margins.label"}],"blocks":[{"type":"@app"}],"presets":[{"name":"t:sections.apps.presets.name"}]}
{% endschema %}

Steps taken to reproduce

  1. Copy dawn theme from Shopify/Dawn into source folder
  2. Run pnpm build or syncify --build --prod --clean or syncify your-shopify-store --theme dev --watch --hot
WolfGreyDev commented 2 months ago

Additionally, if I were to use a shared schema approach to this block, the settings key isn't added to the schema.

I'm unsure if this is intended but it seems that shared schemas aren't checked the same way as the regular schema is when it comes to this issue.

Example

schema/app.schema

{
  "app_block": [
    {
       "type": "@app"
    }
  ]
}

sections/apps.liquid

<div class="{% if section.settings.include_margins %}page-width{% endif %}{% if settings.animations_reveal_on_scroll %} scroll-trigger animate--slide-in{% endif %}">
  {%- for block in section.blocks -%}
    {% render block %}
  {%- endfor -%}
</div>

{% schema %}
{
  "name": "t:sections.apps.name",
  "tag": "section",
  "class": "section",
  "settings": [
    {
      "type": "checkbox",
      "id": "include_margins",
      "default": true,
      "label": "t:sections.apps.settings.include_margins.label"
    }
  ],
  "blocks": [
    {
      "$ref": "app.app_block"
    }
  ],
  "presets": [
    {
      "name": "t:sections.apps.presets.name"
    }
  ]
}
{% endschema %}

Produces

<div class="{% if section.settings.include_margins %}page-width{% endif %}{% if settings.animations_reveal_on_scroll %} scroll-trigger animate--slide-in{% endif %}">
  {%- for block in section.blocks -%}
    {% render block %}
  {%- endfor -%}
</div>

{% schema %}
{"name":"t:sections.apps.name","tag":"section","class":"section","settings":[{"type":"checkbox","id":"include_margins","default":true,"label":"t:sections.apps.settings.include_margins.label"}],"blocks":[{"type":"@app"}],"presets":[{"name":"t:sections.apps.presets.name"}]}
{% endschema %}
panoply commented 2 months ago

Fixed in RC branch: https://github.com/panoply/syncify/tree/rc1

panoply commented 2 months ago

CC @taksh108 who figured out the problem. <3

WolfGreyDev commented 2 months ago

This isn't fixed. You've just removed the @app block 😆

if (block.type === '@app') continue

Should be (I think)

if (block.type === '@app') {
    blocks.push(block);
    continue
}