Query parameter values should be URL encoded

[sherwinski]

Description

The buildURL and buildSrcSet functions should URL encode the values of any query parameters except when a base64 parameter is used.

Example:

import ImgixClient from '@imgix/js-core';

let ub = new ImgixClient({
  domain: 'assets.imgix.net',
})
console.log(
  ub.buildURL(
    "unsplash/walrus.jpg",
    {
      txt: "test!(')",
      "txt-color": "000",
      "txt-size": 400,
      "txt-font": "Avenir-Black",
      "txt-x": 800,
      "txt-y": 600
    }
  )
);

Expected: https://assets.imgix.net/unsplash/walrus.jpg?txt=test!(%27)&txt-color=000&txt-size=400&txt-font=Avenir-Black&txt-x=800&txt-y=600

Actual: https://assets.imgix.net/unsplash/walrus.jpg?txt=test!(')&txt-color=000&txt-size=400&txt-font=Avenir-Black&txt-x=800&txt-y=600

[FilipePfluck]

can I try to work on this one?

[sherwinski]

@FilipePfluck Yes please go for it! I'll mark this issue as assigned to you.

In terms of next steps, please either comment here or open a RFC (request for comment) draft PR explaining the change you'd like to implement.

You can also take a look at our contributing guidelines to get you started.

If you have any trouble at all getting setup or have any questions, don't hesitate to comment here and tag a team member.

[FilipePfluck]

@sherwinski I have a question, I've forked and cloned the repo, and I'm checking out the code right now. But how do I run it, to test what I'm making?

[sherwinski]

Hey @FilipePfluck,

Best recommendation I have is to create tests in either the pathEncoding or buildURL test files and run npm run test to run them.

If you haven't already, please take a moment to read through our contributing docs which will walk you through suggested steps to take when opening a PR.

Please let me know if you have any other questions I can help with.

[FilipePfluck]

It seems like the encodeURIComponent() doesn't encode some characters such as -_.!~*'(). I don't know if I'm understanding it correctly, but I think they're not suposed to be encoded at all.

@sherwinski can you explain to me why do we need to encode the ' character (and perhaps the others I listed above?). If we really need to encode them, I can work on something to encode them aswell.

[sherwinski]

@FilipePfluck good question, let me circle back with the team that originally reported this issue and I'll get back to you.

[sherwinski]

Hey @FilipePfluck and thanks for your patience on this.

After discussing it with the team, we’ve agreed that the best solution for this would be to extend the buildURL and buildSrcSet functions to accept a custom encoder, which allows users to specify their own desired encoding scheme.

The best place to add this would most likely be in the options parameter in each function signature:

const ImgixClient = require("@imgix/js-core");
const client = new ImgixClient({
  domain: 'test.imgix.com',
  secureURLToken: 'xxxxxxxx',
});

client.buildURL(
  "https://imgix-proxy.n8s.jp/img_nikkei-sum.jpg",
  {
    "txt": "test!()*"
  },
  {
    encode: (path) => encodeURI(path).replace("'", "%27")
  }
)

The specified function should be passed down to _sanitizePath and _buildParams , otherwise encoding should just default to encodeURIComponent as it currently does. This should allow users to have more freedom in specifying how URLs get encoded, rather than trying to adjust this library's default behavior to account for every edge case.

[FilipePfluck]

@sherwinski thanks for the return! Understood the idea, and I'll try to work on this when I can

[luqven]

I'm going to re-open this since we should probably allow for Base64 parameters to not get re-encoded when custom encoder() fn is provided. https://github.com/imgix/js-core/blob/02fab156fad0914a8ff5582b706150c96a0b1a24/src/index.js#L122-L125

Open to PRs, otherwise, we'll tackle this at some point next year.

[luqven]

Resolved by #350