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

URI constructor does not take an object as value for query property #366

Closed
wachunei opened this issue Feb 9, 2018 · 5 comments
Closed

Comments

@wachunei
Copy link
Contributor

wachunei commented Feb 9, 2018

Hello everyone,

Please let me know if this is this has been decided to be like this or not, but when building an URL, query property only takes a string as value, instead of search() or query() that take both string and objects.

Heres an example code (online repl here):

const assert = require('assert');
const URI = require('urijs');

const PROTOCOL = 'http';
const HOST = 'mycustom.domain.com';
const PORT = '443';
const BASE_PATH = '/some//path/';
const resource = '///resource//5';
const query = {
  myQuery: 'rocks'
}

const constructorURI = new URI({
    protocol: PROTOCOL,
    hostname: HOST,
    port: PORT,
    path: `${BASE_PATH}/${resource}/`,
    query,
  }).normalize();
  
  
const searchURI = new URI({
    protocol: PROTOCOL,
    hostname: HOST,
    port: PORT,
    path: `${BASE_PATH}/${resource}/`,
  }).search(query).normalize();

assert(constructorURI.toString() === searchURI.toString(), 'URIs are not equal');
@rodneyrehm
Copy link
Member

Hey there!

Providing an object was originally meant to be used internally for convenient cloning. But since it's public API I guess we may want to make this a little more consistent.

You could extend .href() to not assign query to ._parts directly. After all the remaining properties have been processed you could call .query(value, build) and thus allow passing in an object. It's important to call .query() after ._parts has been populated in order for the properties duplicateQueryParameters and escapeQuerySpace to have been imported properly.

With this change you'd also be able to pass URI({ query: '?some=data' }), which, according to the docs, shouldn't be supported right now either.

I guess this change should not only apply to query, but also to fragment in order to properly support the fragment abuse plugins.

Would you like to submit a PR for this change?

@wachunei
Copy link
Contributor Author

wachunei commented Feb 9, 2018

I'm glad to actually find an opportunity to contribute 😄

Will look into this in the next few days, In case of any doubt I'll comment here, if you don't mind.

edit: if you want you can assign it to me ;)

@wachunei
Copy link
Contributor Author

I was a bit lost, I don't know if what I made makes total sense, let me know if I need to fix something.

rodneyrehm pushed a commit that referenced this issue Feb 10, 2018
@wachunei
Copy link
Contributor Author

closed by #367

@rodneyrehm
Copy link
Member

released as v1.19.1

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

No branches or pull requests

2 participants