[fixed] Path.withQuery strips query if query is empty

[mhils]

Hi there,

Path.withQuery did not strip off the query once it became empty. Here's a fix + test. 😃

Cheers,
Max

[ryanflorence]

so would you never want foo/bar/?a?

[mhils]

Maybe if I pass "" instead of undefined, but I feel there should be some way to remove the element from the string.

Whatsoever, that's a different (and maybe opinionated) question. The current behaviour just isn't consistent:

> Path.withQuery('/a/b/c?foo=42&bar=43', { bar: undefined })
'/a/b/c?foo=42'

> Path.withQuery('/a/b/c?foo=42', { foo: undefined })
'/a/b/c?foo=42'

This fix treats the last remaining parameter similar to all others.

Cheers,
Max

[mjackson]

Thanks @mhils !

[mjackson]

@mhils Your test failed. If you'd like I'd be happy to accept a PR where the test passes. Thanks!

[mhils]

@mjackson, tests work fine for me. Seems like travis is having an issue?
https://travis-ci.org/rackt/react-router/builds/43891839
I can't restart the test as it's not my repo.

Cheers,
Max

[mhils]

Ok, looks like there's a bug in the latest version of qs. I now get an error after updating.

> merge({a:'b'}, {a:undefined})
{ a: 'b' }

[mjackson]

@mhils Ah, that's the cause. This definitely seems like something that should be changed upstream in qs. If they won't fix it we may decide to work around it here.

[mhils]

For the record, the patch here needs to be applied anyway. When I fixed this, qs worked as expected. Now, this first needs to be fixed in qs before it can be fixed in react-router... (In other words; why didn't you merge it right away when qs was still working? :D)

[mhils]

@nlf indicated in ljharb/qs#67 that this is not intended usage at all for the merge function. What's your take on this? I can adjust this patch to use object-assign (or something similar) if that would fit for you.