[Theme] Fix local asset serve for backward compatibility #4624
Conversation
frandiox
requested review from
jantnovi and
amcaplan
and removed request for
a team
|
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
Coverage report
Test suite run success1943 tests passing in 876 suites. Report generated by 🧪jest coverage report action from bd948c5 |
There was a problem hiding this comment.
Thank you for this PR, @frandiox! 🔥
--
I guess we can keep supporting it for backward compatibility? @karreiro @mgmanzella
+1 for keeping backward compatibility.
Note: this only work for local assets. We are not forwarding this type of request to the CDN for files that are only present in the remote version or that need to be rendered remotely (e.g.
assets/file.css.liquid). Should we do that?
No, I don't think we should do that. I believe our compass in this case should be backward compatibility, and the legacy version just points that kind of asset directly to the store/Shopify domain, so there's no backward compatibility requirement in this space.
--
Thanks again for this PR! I've left only one minor suggestion about theme app extensions 🚀
WHY are these changes introduced?
Fixes #4607
WHAT is this pull request doing?
Makes the
/cdn/...pathname prefix optional so that it serves local files fromlocalhost:1234/assets/...directly. This is not used by the CLI itself but some users relied on this for custom integrations. I guess we can keep supporting it for backward compatibility? @karreiro @mgmanzellaNote: this only work for local assets. We are not forwarding this type of request to the CDN for files that are only present in the remote version or that need to be rendered remotely (e.g.
assets/file.css.liquid). Should we do that?How to test your changes?
Check that your local theme files can be accesses in the browser from
localhost:1234/assets/...Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist