feat: allow path encoding to be disabled

frederickfogerty commented 2 years ago

Before this PR, this library would encode all paths passed to either buildURL or buildSrcSet. This was a great default in a lot of scenarios, but this proved to be irritating when integrating with some data sources that provided already encoded URL paths. Thus, this PR allows this path encoding behavior to be disabled in the options parameter for both methods.

commit-lint[bot] commented 2 years ago

Tests

path encoding can be disabled (63d4c27)
tsd: path encoding can be disabled (528f14c)
slash is prepended when path encoding is disabled (f65bd76)
signing works as expected when path encoding is disabled (fa491d7)

Features

allow path encoding to be disabled (0bec6fd)

Documentation

info about disabling path encoding (340c86a)

Bug Fixes

ensure slash always between domain and path when sanitizing (e0ead05)

Contributors

frederickfogerty

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR: - `@Commit-Lint merge patch` will merge dependabot PR on "patch" versions (X.X.Y - Y change) - `@Commit-Lint merge minor` will merge dependabot PR on "minor" versions (X.Y.Y - Y change) - `@Commit-Lint merge major` will merge dependabot PR on "major" versions (Y.Y.Y - Y change) - `@Commit-Lint merge disable` will desactivate merge dependabot PR - `@Commit-Lint review` will approve dependabot PR - `@Commit-Lint stop review` will stop approve dependabot PR

frederickfogerty commented 2 years ago

Current dependencies on/for this PR:

main
- PR #314 (this PR)

This comment was auto-generated by Graphite.

sherwinski commented 2 years ago

@frederickfogerty I just realized that this change is probably not compatible with URL signing as it relies on the path being sanitized/encoded first.

frederickfogerty commented 2 years ago

@frederickfogerty I just realized that this change is probably not compatible with URL signing as it relies on the path being sanitized/encoded first.

I'm not sure I'm quite picking up what you're putting down here. What are the implications of this? Does this mean that URL signing can't be used with already encoded URLs?

frederickfogerty commented 2 years ago

@sherwinski I made the trailing slash fix/update here, btw

frederickfogerty commented 2 years ago

@sherwinski just wanted to follow up here if there was any more information about this issue

sherwinski commented 2 years ago

@frederickfogerty I haven't had chance to dig in more but I should have more time today to build out an example that confirms/denies my suspicion. I'll follow up

sherwinski commented 2 years ago

Does this mean that URL signing can't be used with already encoded URLs?

Yeah I'm saying it won't work unless the URL is encoded exactly as we would during the sanitizing step. To me that just seems like a bit of smell since from a user's perspective, it might not be that apparent that path encoding and URL signing are that tightly coupled.

For example, this test will fail (the expected value includes the signature if path encoding were true):

const client = new ImgixClient({
  domain: 'sdk-test.imgix.net',
  includeLibraryParam: false,
  secureURLToken: 'abc123',
});

const actual = client.buildURL(
  '/file+with%20some+crazy?things.jpg',
  {
    w: 100,
  },
  {
    disablePathEncoding: true,
  },
);

const expected =
  'https://sdk-test.imgix.net/file%2Bwith%2520some%2Bcrazy%3Fthings.jpg?w=100&s=0c4068b0df31a8a8b67dd79247063939';

assert.strictEqual(actual, expected);

Not sure if the best path here is to document this as a caveat or throw a warning if users attempt to sign a URL while also disabling the encoding step.

imgix-git-robot commented 2 years ago

:tada: This PR is included in version 3.5.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

imgix / js-core