Skip to content

Conversation

robertjd
Copy link
Member

Fixes stormpath/express-stormpath#438

@typerandom so here's the deal: the cloneextend module was calling hasOwnProperty() directly on objects, and that is brittle. Why this became a problem in node v6 I'm not sure and I didn't dig into it. But someone did fix this issue here, by using a more robust method for checking an object for properties:

https://github.com/shimondoodkin/nodejs-clone-extend/pull/5/files

Using that commit does solve the problem in stormpath/express-stormpath#438. That library isn't maintained very well, e.g. master has breaking changes in it, and package.json has a bumped minor number, which isn't even in npm anyways. As such, I'm moving the author's code into this library so that we aren't depending on a fragile library.

Other changes in this PR:

  • The "JSON syntax error" message text has changed in v6, so I had to update the test assertion accordingly
  • Adding node v6 to travis

- Updating a test validation message to match the new message format

- Moving “clone-extend” into this library, at a specific commit, to resolve issues with calling `hasOwnProperty`

- Adding node v6 to travis
@coveralls
Copy link

coveralls commented Aug 11, 2016

Coverage Status

Coverage decreased (-11.7%) to 77.062% when pulling affda40 on node-v6-fixes into 8c92697 on master.

@typerandom
Copy link
Contributor

typerandom commented Aug 11, 2016

I have tested and reviewed this and I'm happy with the change. I don't like inlining that patched clone-extend like that. But it's really one of the better alternatives right now, so I'm ok with it. We've had quite a bit of issues with it, so another solution to avoid inlining would be to create a separate package and also add tests for it so that we know that the ways that we want to extend functions as we expect them to.

@typerandom typerandom merged commit eb473f8 into master Aug 11, 2016
@typerandom typerandom deleted the node-v6-fixes branch August 11, 2016 10:13
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.

Crashes on load in node 6.2

3 participants