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

fix: follow allowPrototypes during merge #201

Merged
merged 1 commit into from Mar 6, 2017

Conversation

dead-horse
Copy link
Contributor

@dead-horse dead-horse commented Mar 2, 2017

closes #200

@dead-horse
Copy link
Contributor Author

dead-horse commented Mar 2, 2017

I'm not sure what's the purpose of this commit 50ea161 , but I think it didn't fix prototype overwrite.

@ljharb
Copy link
Owner

ljharb commented Mar 2, 2017

The purpose of the commit is in the commit message, and the added tests shows exactly what it inarguably did fix.

Copy link
Owner

@ljharb ljharb left a comment

Change looks good, just a few comments.


st.deepEqual(
qs.parse('a[b]=c&a=toString', { allowPrototypes: true }),
{ a: { b: 'c', toString: true } },
Copy link
Owner

@ljharb ljharb Mar 2, 2017

Choose a reason for hiding this comment

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

in this case, shouldn't a[b] be entirely overwritten by the string "toString"?

Copy link
Contributor Author

@dead-horse dead-horse Mar 3, 2017

Choose a reason for hiding this comment

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

I found this test case t.deepEqual(qs.parse('a[b]=c&a=d'), { a: { b: 'c', d: true } }, 'can add keys to objects');, is this a feature by design?

Copy link
Owner

@ljharb ljharb Mar 3, 2017

Choose a reason for hiding this comment

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

good point :-) let's keep this as-is for now.

dist/qs.js Outdated
});
Copy link
Owner

@ljharb ljharb Mar 2, 2017

Choose a reason for hiding this comment

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

this file shouldn't be updated in PRs; please revert it.

Copy link
Contributor Author

@dead-horse dead-horse Mar 3, 2017

Choose a reason for hiding this comment

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

fixed

@dead-horse
Copy link
Contributor Author

dead-horse commented Mar 3, 2017

after 50ea161 ,

qs.parse('[=a'); // { a: true }
qs.parse(']=a'); // { ']': 'a'}

Is this the expected result?

@dead-horse
Copy link
Contributor Author

dead-horse commented Mar 3, 2017

if we revert the change of var parent = /^([^[\]]*)/; to var parent = /^([^\[\]]*)/;, the only failing test case is:

not ok 135 should be equivalent
  ---
    operator: deepEqual
    expected: |-
      { ']': 'toString' }
    actual: |-
      {}
    at: Test.<anonymous> (/Users/deadhorse/git/github.com/ljharb/qs/test/parse.js:441:12)
  ...

@ljharb
Copy link
Owner

ljharb commented Mar 3, 2017

@dead-horse the second line in your example is the expected result, which is what it did, actually, fix. The first line is an oversight, obviously, hence #200.

@ljharb
Copy link
Owner

ljharb commented Mar 3, 2017

That test is valid; I would expect [=toString to become { '[': 'toString' } as well.

ljharb
ljharb approved these changes Mar 3, 2017
@popomore
Copy link

popomore commented Mar 3, 2017

I want to know the diff in 50ea161?diff=split#diff-e6c9a6bca996bc454cc244d17bfeda5cL125

- st.deepEqual(qs.parse('a[]=b&a[t]=u&a[hasOwnProperty]=c', { allowPrototypes: false }), { a: { '0': 'b', c: true, t: 'u' } });
+ st.deepEqual(qs.parse('a[]=b&a[t]=u&a[hasOwnProperty]=c', { allowPrototypes: false }), { a: { 0: 'b', t: 'u' } });

Is it a expected change? it seems a break change that change string to number.

@ljharb
Copy link
Owner

ljharb commented Mar 3, 2017

@popomore in JS, all keys are strings (or Symbols), so { 0: true } and { '0': true } are identical and indistinguishable.

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

Successfully merging this pull request may close these issues.

3 participants