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

Prefix keys in seqObj's seenKeys to avoid collisions #235

Merged
merged 3 commits into from
Apr 6, 2018

Conversation

jfirebaugh
Copy link
Contributor

Otherwise, seqObj will fail for the keys in Object.getOwnPropertyNames(Object.prototype), the most likely collision being "constructor".

An alternate implementation strategy is Object.create(null), but it's not supported on IE <9, and is not possible to polyfill.

Otherwise, seqObj will fail for the keys in `Object.getOwnPropertyNames(Object.prototype)`, the most likely collision being "constructor".

An alternate implementation strategy is `Object.create(null)`, but it's not supported on IE <9, and is not possible to polyfill.
@wavebeem
Copy link
Collaborator

wavebeem commented Apr 4, 2018

good catch, thanks. and appreciate the eye for ie compat!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 45acb6a on mapbox:seqObj-special-keys into 65022cb on jneen:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 45acb6a on mapbox:seqObj-special-keys into 65022cb on jneen:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 45acb6a on mapbox:seqObj-special-keys into 65022cb on jneen:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 45acb6a on mapbox:seqObj-special-keys into 65022cb on jneen:master.

@coveralls
Copy link

coveralls commented Apr 4, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling c506e0d on mapbox:seqObj-special-keys into 65022cb on jneen:master.

@wavebeem
Copy link
Collaborator

wavebeem commented Apr 4, 2018

wow idk what's going on with coveralls there lol.

i'll merge this and release when i get some free time.

@wavebeem
Copy link
Collaborator

wavebeem commented Apr 5, 2018

This solution should be good for 99% of cases but I think it would be more correct to use hasOwnProperty for this check since technically someone could add $constructor or similar to Object.prototype.

hasOwnProperty goes back to ES3 so it should work in all environments Parsimmon supports.

Also, would you mind fixing this in bitSeqObj too?

@wavebeem wavebeem merged commit 06713f6 into jneen:master Apr 6, 2018
@wavebeem
Copy link
Collaborator

wavebeem commented Apr 6, 2018

Thanks for the bugfixes!

@jfirebaugh jfirebaugh deleted the seqObj-special-keys branch April 6, 2018 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants