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

Remove "clever" decoding from _sanitizePath #29

Merged
merged 1 commit into from
Aug 28, 2017
Merged

Remove "clever" decoding from _sanitizePath #29

merged 1 commit into from
Aug 28, 2017

Conversation

jayeb
Copy link
Contributor

@jayeb jayeb commented Aug 25, 2017

I had previously updated the ImgixClient._sanitizePath() method in an attempt to be clever and properly handle pre-encoded paths. This introduced unnecessary complexity into the method (detemining whether or not a given string is unencoded, sorta pre-encoded, or fully pre-encoded is trickier than it seems at first glance) and introduced stupid bugs into its base use-case.

Having failed my attempt at cleverness, I am suitably chagrined. This PR removes all code dealing with pre-encoded paths from ImgixClient._sanitizePath() so it only handle non-encoded inputs. Our README has instructed users to only pass in non-encoded data from the start:

Whenever you provide data to imgix-core-js, make sure it is not already URL-encoded, as the library handles proper encoding internally.

This also fixes #28, and includes a new test case to ensure non-URL paths with spaces are handled properly.

@oaleynik Would you care to review these changes for me?

@oaleynik
Copy link
Contributor

@jayeb thanks for fixing that! I guess the problem we had previously in #7 was due to the use of encodeURIComponent for the path part instead of encodeURI. encodeURI should be handling reserved characters appropriately (per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURI), so I think this is good to go.

@jayeb jayeb merged commit 1b04bd7 into master Aug 28, 2017
@jayeb jayeb deleted the fix-28 branch August 28, 2017 15:12
@jayeb
Copy link
Contributor Author

jayeb commented Aug 28, 2017

I'll be releasing this as 1.1.0. While this is sort of a patch-level change in that it's fixing a bug, it is technically changing the interface of ImgixClient._sanitizePath(). Even though it's changing the interface to more closely match the documented behavior, I'd rather push this as a minor-level change just in case someone somewhere had an implementation that was dependent on the non-documented behavior of that method.

@oaleynik
Copy link
Contributor

@jayeb that makes perfect sense. Thanks again for fixing this quickly! 🤘🏻

@ericdeansanchez
Copy link
Contributor

🎉 This PR is included in version 0.3.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

If filename in path has spaces - signature becomes invalid
3 participants