Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Really odd behaviour with image_url output as 'data-...' attribute on Shopify theme dev server #2240

Closed
2 tasks done
twilson90 opened this issue Jun 27, 2023 · 7 comments · Fixed by #2285
Closed
2 tasks done
Labels
Area: @shopify/theme @shopify/theme package issues Type: Bug Something isn't working

Comments

@twilson90
Copy link

twilson90 commented Jun 27, 2023

Please confirm that you have:

  • Searched existing issues to see if your issue is a duplicate. (If you’ve found a duplicate issue, feel free to add additional information in a comment on it.)
  • Reproduced the issue in the latest CLI version.

In which of these areas are you experiencing a problem?

Theme

Expected behavior

{% liquid
  assign url1 = image | image_url: width: settings.max_image_size
  assign url2 = image | image_url: width: image_size
%}
<a
  href="{{ url1 | escape }}"
  class="thumbnail"
  data-img="{{ url2 | escape }}"
  data-width="{{ image.width }}"
  data-height="{{ image.height }}">
  {%- render "image" with image: image, width: 60, placeholder: true, loading: "eager" -%}
</a>

Something bizarre is happening with this code when I try and test it locally.

The href tag (url1) will become //{my-shop-url}/cdn/shop/products/image.jpg?width=1280
However the data-img tag (url2) becomes http://127.0.0.1:9292/cdn/shop/products/image.jpg?width=720

This url if I try to load it doesn't work, as it appears no CDN urls work in the local test environment and have to loaded from the test site directly.

Yet if I simply print url2 outside of an attribute tag it correctly prints //{my-shop-url}/cdn/shop/products/image.jpg?width=720

What appears to be happening is the liquid processor, after substituting all liquid tags with their resulting outputs, then runs a kind of hacky 'find and replace' on all instances of http://127.0.01/ with //{my-shop-url}/, seemingly in an effort to resolve a recently 'fixed' bug mentioned in these issues (one of which I reported):
#2105
#1859
But for some reason it specifically ignores replacing 'data-...' attributes. Using any attribute name other than 'data-...', the url is correctly output.

There seems to be some very questionable code maintenance happening behind the scenes if my assumption is correct, it all seems very hacky to me.

Actual behavior

Verbose output

-

Reproduction steps

Operating System

Windows 11

Shopify CLI version (check your project's package.json if you're not sure)

3.46.5

Shell

Windows Command Prompt

Node version (run node -v if you're not sure)

18.2.0

What language and version are you using in your application?

No response

@twilson90 twilson90 added the Type: Bug Something isn't working label Jun 27, 2023
@ttnbtfy
Copy link

ttnbtfy commented Jun 29, 2023

same here
Current Shopify CLI version: 3.47.0

Imgur

@knjshimi
Copy link

Same here after upgrading to Shopify CLI v3.47.0

@isaacroldan isaacroldan added the Area: @shopify/theme @shopify/theme package issues label Jul 3, 2023
@isaacroldan
Copy link
Contributor

@karreiro This might be related to a recent change?

@MaxDesignFR
Copy link

Same for me. I suppose this is due to recent changes to loading image assets, I never had an issue before that.

This liquid snippet:

{{- img | image_url: width: img.width | image_tag:
     loading: 'lazy',
     class: 'my_class',
     width: img.width,
     height: img.height,
     alt: img.alt | escape
-}}

Will render in local server as:

<img src="/assets/banner-maxdesign.js?v=166105568692117195741688481357">

So the actual image source and data attributes have been removed, and the source is a script asset that only comes later in the code. So there is definitely something fishy here, because not only does this not load the correct src, but it also breaks the html. I have something like this in view-source and you can notice how the image tag loads a script asset that is suppose to come later, picture tag is not closed, closing script tag out of nowhere:

<html>
<body>
<!--StartFragment-->

<picture>
--
  | <source
  | media="(min-width:740px)"
  | srcset="//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048 1800w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=375 375w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=500 500w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=750 750w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=1000 1000w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=1250 1250w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=1500 1500w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=1750 1750w"
  | sizes="max(900px, min(100vw, 0px))">
  | <source
  | media="(max-width:739px)"
  | srcset="//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048 1800w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=375 375w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=500 500w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=750 750w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=1000 1000w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=1000 1250w,//japon-avenue.myshopify.com/cdn/shop/files/Sans_titre_44.jpg?v=1671019048&width=1500 1500w"
  | sizes="100vw"><img src="/assets/banner-maxdesign.js?v=166105568692117195741688481357"></script></section>

<!--EndFragment-->
</body>
</html>

And for reference, this is how the (simplified) liquid looks like in my use case:

<banner-maxdesign>
   <div>
      <picture>
          <source
            media="(min-width:740px)"
            srcset="{{ desktop.img | image_url }} {{ desktop.img.width }}w,
              {%- if desktop.img.width >= 375 -%}{{ desktop.img | image_url: width: 375 }} 375w{%- endif -%}
              {%- if desktop.img.width >= 500 -%},{{ desktop.img | image_url: width: 500 }} 500w{%- endif -%}
              {%- if desktop.img.width >= 750 -%},{{ desktop.img | image_url: width: 750 }} 750w{%- endif -%}
              {%- if desktop.img.width >= 1000 -%},{{ desktop.img | image_url: width: 1000 }} 1000w{%- endif -%}
              {%- if desktop.img.width >= 1250 -%},{{ desktop.img | image_url: width: 1250 }} 1250w{%- endif -%}
              {%- if desktop.img.width >= 1500 -%},{{ desktop.img | image_url: width: 1500 }} 1500w{%- endif -%}
              {%- if desktop.img.width >= 1750 -%},{{ desktop.img | image_url: width: 1750 }} 1750w{%- endif -%}
              {%- if desktop.img.width >= 2000 -%},{{ desktop.img | image_url: width: 2000 }} 2000w{%- endif -%}
              {%- if desktop.img.width >= 2500 -%},{{ desktop.img | image_url: width: 2500 }} 2500w{%- endif -%}
              {%- if desktop.img.width >= 3000 -%},{{ desktop.img | image_url: width: 3000 }} 3000w{%- endif -%}"
            sizes="{{ desktop_sizes }}">
          <source
            media="(max-width:739px)"
            srcset="{{ mobile_img | image_url }} {{ mobile_img.width }}w,
              {%- if mobile_img.width >= 375 -%}{{ mobile_img | image_url: width: 375 }} 375w{%- endif -%}
              {%- if mobile_img.width >= 500 -%},{{ mobile_img | image_url: width: 500 }} 500w{%- endif -%}
              {%- if mobile_img.width >= 750 -%},{{ mobile_img | image_url: width: 750 }} 750w{%- endif -%}
              {%- if mobile_img.width >= 1000 -%},{{ mobile_img | image_url: width: 1000 }} 1000w{%- endif -%}
              {%- if mobile_img.width >= 1250 -%},{{ mobile_img | image_url: width: 1000 }} 1250w{%- endif -%}
              {%- if mobile_img.width >= 1500 -%},{{ mobile_img | image_url: width: 1500 }} 1500w{%- endif -%}"
            sizes="{% if mobile.media_height != 0 %}{{ mobile.media_height | times: mobile_img.aspect_ratio | round }}px{% else %}100vw{% endif %}">
          {{- desktop.img | default: mobile.img | image_url: width: desktop.img.width | image_tag:
            loading: 'lazy',
            width: desktop.img.width,
            height: desktop.img.height,
            alt: desktop.img.alt | escape
          -}}
        </picture>
    </div>
</banner-maxdesign>

{%- # region(collapsed) JS ASSETS -%}
{%- if mobile.media_type == 'video' or desktop.media_type == 'video' -%}
  {%- capture script -%}banner-maxdesign{% if settings.md_minified_scripts %}.min{% endif %}.js{%- endcapture -%}
  <script src="{{ script | asset_url }}"></script>
{%- endif -%}
{%- # endregion -%} 

@hoanganhover
Copy link

hoanganhover commented Jul 6, 2023

Same for me

I have updated to the latest version 3.47.3
but still error
And error on windows
However, my mac still works

@kevin-aminzadeh
Copy link

Experiencing the same issue here as of the last 24 hours with no changes to CLI version in the project. As part of the debugging process I've tried versions 3.46.1, 3.47.2 and 3.47.3 with all versions failing to load image assets.

Operating System
Windows 11

Shopify CLI version
3.46.1, 3.47.2 and 3.47.3

Shell
Gitbash

Node version
v20.3.0

@ceri-waterscreative
Copy link

Same problem here.

Operating System
Windows 11

Shopify CLI version
3.47.3

Shell
Gitbash

Node version
v18.12.1

Some images are coming back with 404s others are actually coming back corrupted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: @shopify/theme @shopify/theme package issues Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants