Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Fix errors on section rendering caused by CORS issues #2078

Merged
merged 3 commits into from
Feb 22, 2022
Merged

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Feb 21, 2022

WHY are these changes introduced?

The Dawn 1.1.0 theme relies on data attribute values to re-render sections with JavaScript:

Screenshot 2022-02-20 at 11 25 17

However, when users change product variants, the exact error reported by #1466 happens, as the client-side request fails due to CORS issues.

Regarding my other (reverted) PR

My other PR (#1952) was actually fixing a different problem with less impact. It handled 307 HTTP requests with CORS errors happening only on Safari with some assets.

Those assets do not impact section rendering. So, I've created Shopify/cli#2001 for handling this scenario.

WHAT is this pull request doing?

This PR adopts a similar approach from the other middlewares and replaces shop URLs from templates, pointing them to 127.0.0.1:9292.

How to test your changes?

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

Before this PR:
before

After this PR:
after

Post-release steps

None.

Update checklist

  • I've added a CHANGELOG entry for this PR (if the change is public-facing)
  • I've considered possible cross-platform impacts (Mac, Linux, Windows).
  • I've left the version number as is (we'll handle incrementing this when releasing).
  • I've included any post-release steps in the section above.

@karreiro karreiro requested review from macournoyer, a team, hannachen and amcaplan and removed request for a team February 21, 2022 11:12
Copy link
Contributor

@macournoyer macournoyer left a comment

Choose a reason for hiding this comment

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

As this will replace ALL urls on the page, I wasn't sure if that was going to work with cart & checkout, but after some quick testing it seem to work (I did not test completing a full checkout flow, just submitting the form). 🙌

lib/shopify_cli/theme/dev_server/proxy.rb Outdated Show resolved Hide resolved
@karreiro
Copy link
Contributor Author

Thanks for the review, @macournoyer! I've realized the regex could be more specific for solving the issue. So, I've changed it to handle only URLs that appear on data attributes.

In the future, if we feel we need to cover broader scenarios, we may make it more generic, but being specific seems safer now :)

Thanks again for the review! 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Section Rendering fails due to lack of CORS Access-Control-Allow-Origin header
2 participants