-
Notifications
You must be signed in to change notification settings - Fork 130
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
[Theme] Fix serving local assets that contain non-printable characters #4494
Conversation
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 success1890 tests passing in 858 suites. Report generated by 🧪jest coverage report action from 0a6475b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM! Curious why we have to figure out the content-length for our server. That seems like a low level detail that the server lib usually provides for us.
(I'm really out of the loop here, so if what we're doing is is pretty low level, I understand!)
Those URL regex do be gnarly af though. Took me a couple of sips of coffee to get why they were modified :P
Thanks for the review! Yeah we are using a lowish level server called
Yep, just simplifying them a bit 😅 -- I think we'll probably forget about RegExps and just ask AI to change them for us 😂 |
/snapit |
🫰✨ Thanks @frandiox! Your snapshot has been published to npm. Test the snapshot by intalling your package globally: pnpm i -g @shopify/[email protected]
|
WHY are these changes introduced?
Closes #4479
WHAT is this pull request doing?
The main issue was caused by non-printable characters, which have a different length in disk vs in memory (string format). By transforming them to Buffer before reading the length we get the real disk value, which is required for
content-length
header.Unrelated fixes: consider replacing CDN links in JS files with single quotes, etc.
How to test your changes?
Added a unit test to show it.
cc @Shopify/advanced-edits