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

Improve compatibility with Node.js v6.0.0 #104

Merged
merged 1 commit into from
May 8, 2016

Conversation

jackhsu978
Copy link
Contributor

querystring in node v6 now returns an "empty" object instead of {}, causing an error: “TypeError: parsedQuery.hasOwnProperty is not a function”. Instead calling hasOwnProperty on parsedQuery, just check parsedQuery['_escaped_fragment_'] instead, as suggested by @thoop

@thoop
Copy link
Contributor

thoop commented Apr 28, 2016

Weird I wouldn't expect those tests to fail just because of that change. I'll have to look into this unless you have any ideas what caused that.

@jackhsu978
Copy link
Contributor Author

@thoop I don't know why either. I ran npm test locally and everything is passing.

@jackhsu978
Copy link
Contributor Author

@thoop I forgot about the case where the parameter _escaped_fragment_ is an empty string. should be fixed.

`querystring` in node v6 now returns an empty object instead of `{}`,
causing an error: “TypeError: parsedQuery.hasOwnProperty is not a
function”. This patch should fix it.
@jackhsu978 jackhsu978 changed the title Make it work with node v6.0.0 Improve compatibility with Node.js v6.0.0 Apr 29, 2016
@mscdex
Copy link

mscdex commented Apr 29, 2016

The alternative fix is to use something like this:

function hasOwnProperty(obj, prop) {
  return Object.prototype.hasOwnProperty.call(obj, prop);
}

// Then use like:
hasOwnProperty(parsedQuery, '_escaped_fragment_')

which is "safer" since hasOwnProperty() (or any other inherited property) on objects can be overridden anyway.

@thoop
Copy link
Contributor

thoop commented Apr 30, 2016

I don't think we ever want to look up the chain for _escaped_fragment_ so this fix should work fine.

@jackhsu978
Copy link
Contributor Author

@thoop Do you mind merging this? Please let me know if there is any other change that you'd like to see and I will be happy to tweak.

@thoop
Copy link
Contributor

thoop commented May 7, 2016

Sorry! Been really busy this week. I'll merge it this weekend.

@thoop thoop merged commit fb08c9a into prerender:master May 8, 2016
@thoop thoop removed the needs review label May 8, 2016
@thoop
Copy link
Contributor

thoop commented May 8, 2016

Thanks for your help!

@jackhsu978
Copy link
Contributor Author

Thank you for the merge @thoop! 👍

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.

4 participants