-
-
Notifications
You must be signed in to change notification settings - Fork 452
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
Add .stringifyUrl()
method
#217
Add .stringifyUrl()
method
#217
Conversation
index.d.ts
Outdated
|
||
|
||
/** | ||
Stringify an object of URL and the query into a URL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be useful with an example here.
index.js
Outdated
const queryFromUrl = this.extract(input.url); | ||
const hash = getHash(input.url); | ||
const stringifyQuery = this.stringify(input.query, options); | ||
let queryString = [queryFromUrl, stringifyQuery].filter(str => str).join('&'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use acronyms. str
=> string
readme.md
Outdated
@@ -221,6 +221,21 @@ queryString.parseUrl('https://foo.bar?foo=bar'); | |||
//=> {url: 'https://foo.bar', query: {foo: 'bar'}} | |||
``` | |||
|
|||
### .stringifyUrl(object, options?) | |||
|
|||
The inverse of [`.parseUrl()`](https://github.com/sindresorhus/query-string#parseurlstring-options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this, it should describe what it does.
readme.md
Outdated
|
||
The inverse of [`.parseUrl()`](https://github.com/sindresorhus/query-string#parseurlstring-options) | ||
|
||
The `object` are the same as result of `.parseUrl` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easier to just describe what it accepts specifically here.
readme.md
Outdated
|
||
The `options` are the same as for `.stringify()`. | ||
|
||
Returns a URL with query string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returns a URL with query string. | |
Returns a URL with a query string. |
test/stringify-url.js
Outdated
t.deepEqual(queryString.stringifyUrl({url: 'https://foo.bar?', query: {foo: 'bar'}}), 'https://foo.bar?foo=bar'); | ||
t.deepEqual(queryString.stringifyUrl({url: 'https://foo.bar/#top', query: {foo: 'bar'}}), 'https://foo.bar/?foo=bar#top'); | ||
t.deepEqual(queryString.stringifyUrl({url: 'https://foo.bar', query: {foo: 'bar', a: 'b'}}), 'https://foo.bar?a=b&foo=bar'); | ||
t.deepEqual(queryString.stringifyUrl({url: 'https://foo.bar?', query: {foo: ['bar', 'baz']}}), 'https://foo.bar?foo=bar&foo=baz'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a test to show what happens if the URL already has a query string and a query string is specified.
Also add some tests that does parseUrl
and then pass the result to stringifyUrl
to make sure they are compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// @komcal
test/stringify-url.js
Outdated
import test from 'ava'; | ||
import queryString from '..'; | ||
|
||
test('stringify url not containing query string', t => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test('stringify url not containing query string', t => { | |
test('stringify URL without query string', t => { |
test/stringify-url.js
Outdated
t.deepEqual(queryString.stringifyUrl({url: 'https://foo.bar?foo=bar', query: {}}), 'https://foo.bar?foo=bar'); | ||
}); | ||
|
||
test('stringify url with query string', t => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test('stringify url with query string', t => { | |
test('stringify URL with query string', t => { |
@@ -206,3 +206,13 @@ export function stringify( | |||
Extract a query string from a URL that can be passed into `.parse()`. | |||
*/ | |||
export function extract(url: string): string; | |||
|
|||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No many empty lines
@sindresorhus updated |
test/stringify-url.js
Outdated
t.deepEqual(queryString.stringifyUrl({url: 'https://foo.bar/#top', query: {foo: 'bar'}}), 'https://foo.bar/?foo=bar#top'); | ||
t.deepEqual(queryString.stringifyUrl({url: 'https://foo.bar', query: {foo: 'bar', a: 'b'}}), 'https://foo.bar?a=b&foo=bar'); | ||
t.deepEqual(queryString.stringifyUrl({url: 'https://foo.bar?', query: {foo: ['bar', 'baz']}}), 'https://foo.bar?foo=bar&foo=baz'); | ||
t.deepEqual(queryString.stringifyUrl({url: 'https://foo.bar?foo=baz', query: {foo: 'bar'}}), 'https://foo.bar?foo=baz&foo=bar'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be surprising behavior for me. Adding another query item here with the same name actually changes the semantics as now the result would be an array instead of a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sindresorhus you think the result would be https://foo.bar?foo[]=bas&foo[]=bar
? I'm not sure what the answer would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that would be completely wrong as it's a different arrayFormat
format.
I'm saying that it should replace instead: 'https://foo.bar?foo=bar'
. The behavior should be clearly documented.
#217 (comment) is not done. And the English-prose needs more work (typos + could be better written). |
Bump |
@sindresorhus I’m terribly sorry for absent communication. I'm practicing to write English correctly if you have any suggestion about the typo you can guide |
|
||
/** | ||
Stringify an object into a URL with a query string and sorting the keys. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to document the merging behavior here.
index.d.ts
Outdated
``` | ||
*/ | ||
export function stringifyUrl( | ||
input: ParsedUrl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input: ParsedUrl, | |
object: ParsedUrl, |
Thanks for working on this :) |
resolve #205
I create a method named
stringifyUrl
that does the inverse of.parseUrl()