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

Fix image_url on data attributes #2285

Merged
merged 2 commits into from
Jul 11, 2023
Merged

Conversation

montalvomiguelo
Copy link
Contributor

@montalvomiguelo montalvomiguelo commented Jul 1, 2023

WHY are these changes introduced?

Fixes #2240

WHAT is this pull request doing?

This PR reverts the patch_body method since it's not needed anymore to fix CORS issues. Shopify/shopify-cli#2078. It will prevent replacing Shop Uris with Local Uris in data attributes.

Using the current domain addressed CORS issues per Shopify/dawn#848

How to test your changes?

pnpm shopify theme dev --path my-cool-theme

Notice that images load from CDN successfully, like in #2143

Screenshot 2023-07-01 at 12 53 54 PM

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
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

@montalvomiguelo montalvomiguelo force-pushed the proxy-fix branch 2 times, most recently from c12b87e to 8ec7dd0 Compare July 1, 2023 09:06
@montalvomiguelo montalvomiguelo changed the title Revert Shopify/shopify-cli#2078 Fix image_url on data attributes Jul 1, 2023
@isaacroldan isaacroldan requested a review from karreiro July 4, 2023 09:10
Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this PR, @montalvomiguelo!

I've tried the reproduction steps from the previous PR, and noticed that removing patch_body would reintroduce the previous issue:

demo

Here's the steps I've followed:

  • Clone the Dawn theme using git clone git@github.com:Shopify/dawn.git
  • Switch to version 1.1.0 with git reset 2566749e4226818500809892259b7713808319ae --hard
  • Import product data from products.csv into your store
  • Run shopify theme dev
  • Interact with the Classic Varsity Top and observe the browser console

Therefore, perhaps instead of removing patch_body, we could adjust the regex to make it more specific to the data-base-url attribute. This way, we could handle both Shopify/shopify-cli#1466 and #2240. What do you think?

Thanks again for this PR 🚀

@montalvomiguelo
Copy link
Contributor Author

Hi @karreiro (:

It seems like the CORS issue persisted until Dawn v2.3.0 as reported in Shopify/dawn#548. Requesting the resources from the same origin solved the CORS issue per the work in Shopify/dawn#848. Prolly patching the data attributes is not needed anymore.

@karreiro
Copy link
Contributor

karreiro commented Jul 6, 2023

Hey @montalvomiguelo,

That's accurate indeed, but partners who have to deal with outdated Dawn themes would face this issue. With that in mind, I wonder if replacing the url only on data-base-url attributes would be the ideal approach to support both scenarios. What do you think from that perspective?

Thanks again for the awesome PR :) 🚀

@montalvomiguelo
Copy link
Contributor Author

That approach makes sense. Please let me adjust the regex to replace only data-base-url.

Thanks @karreiro (:

Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @montalvomiguelo! 🔥🚀

@karreiro karreiro added this pull request to the merge queue Jul 11, 2023
Merged via the queue into Shopify:main with commit 66b0c71 Jul 11, 2023
@shopify-shipit shopify-shipit bot temporarily deployed to nightly July 11, 2023 15:15 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to production July 26, 2023 11:18 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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