Skip to content

Don't pass 'undefined' for missing optional parameters #4064

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

Merged
merged 3 commits into from
Oct 26, 2016

Conversation

jochenberger
Copy link
Contributor

@jochenberger jochenberger commented Oct 19, 2016

decodeURIComponent(undefined) is "undefined"

decodeURIComponent(undefined) is "undefined"
@jochenberger
Copy link
Contributor Author

See #3970, #3991

location={{ pathname: '/foo' }}
component={(props) => {
expect(props).toEqual({
params: { foo: 'foo', bar: undefined },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do we rather expect { foo: 'foo' } here, without the bar key?

@@ -23,7 +23,7 @@ const getMatcher = (pattern, exactly) => {

const parseParams = (pattern, match, keys) =>
match.slice(1).reduce((params, value, index) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may want to add filter(value => value !== undefined) before the reduce call to prevent "missing" params to be defined as param properties at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter sounds good to me.

@@ -23,7 +23,7 @@ const getMatcher = (pattern, exactly) => {

const parseParams = (pattern, match, keys) =>
match.slice(1).reduce((params, value, index) => {
params[keys[index].name] = decodeURIComponent(value)
params[keys[index].name] = value === undefined ? undefined : decodeURIComponent(value)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and leave this modification out

@jochenberger jochenberger changed the title Don't try do decode undefined Don't pass 'undefined' for missing optional parameters Oct 25, 2016
@jochenberger
Copy link
Contributor Author

PR updated

Copy link
Member

@timdorr timdorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ryanflorence ryanflorence merged commit 812f60e into remix-run:v4 Oct 26, 2016
@msuntharesan
Copy link

Is this getting released soon?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants