fridays / next-routes

Universal dynamic routes for Next.js
MIT License
2.47k stars 230 forks source link

Do not encode null values for params #120

Closed mergebandit closed 6 years ago

mergebandit commented 6 years ago

If you have an object with the following properties:

const myObj = {
  foo: 42,
  bar: null
}

And then use that object in the Link params, like so:

<Link route="fridaysAwesome" params={{ prop.foo, prop.bar }}>...</Link>

The resultant query object passed to components is the following:

//query
{
  foo: 42,
  bar: "null"
}

as a result of encodeURIComponent(null)

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 3980765613cc12336bf251da288f3dfaa4023c0b on mergebandit:master into 944397850cd353fe5fb62f460ecef45ee6543cce on fridays:master.

fridays commented 6 years ago

Thank you, good catch! We should do the same for undefined. And maybe instead of an empty string, remove the key. What do you think?

mergebandit commented 6 years ago

Hey - apologies for delayed response!

Yeah I had undefined in my first near-PR and I removed it. I can't recall why, nor can I think of a good reason to do so.

Added a commit which filters the toQueryString method for values that are null or undefined.

The only question I have is in my test, the as property returns /a/b? - I could add logic to the return of the Route.getAs method to not include the ?, but wasn't sure if that was necessary.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling dce003b2e08a4f5b3ec104812e42feae46dfe5b4 on mergebandit:master into 944397850cd353fe5fb62f460ecef45ee6543cce on fridays:master.

fridays commented 6 years ago

Thank you, that's fine!

Published in v1.1.1