Query parameter values should be URL encoded about js-core HOT 10 CLOSED

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

from js-core.

can I try to work on this one?

from js-core.

@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.

from js-core.

@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?

from js-core.

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.

from js-core.

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.

from js-core.

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

from js-core.

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.

from js-core.

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.

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

from js-core.

Resolved by #350

from js-core.