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 hasOwnProperty is not a function error #430

Merged
merged 4 commits into from
Jan 24, 2018

Conversation

Pritilender
Copy link
Contributor

Writing koa adapter, I came accros an error when I pass ctx.response to make a new Response object which stated that options.headers.hasOwnProperty is not a function.
I've found that I'm not alone and that there was an issue: #413 (which, btw, is closed, but don't know why).

The origin of this error could be that somewhere in koa, or wherever, headers are created using Object.create(null).
This PR changes the way of calling hasOwnProperty so even if an object is created with Object.create(null), we can still check for own properties.

Also, there are two more places where hasOwnProperty is called as a prototype function of the object, not as direct call to prototype function, but I haven't changed them.
Should I changed them also or no?

@Pritilender
Copy link
Contributor Author

Oh and also because I have the latest npm (v5 something), it created package-lock.json automatically, so I think it's better to include it in the project also.

Copy link
Member

@maxtruxa maxtruxa left a comment

Choose a reason for hiding this comment

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

I'm all for calling Object#hasOwnProperty explicitly.

But could you submit a new PR for the package-lock.json so that the change can be reviewed and discussed separately?

@maxtruxa
Copy link
Member

@Pritilender Could you change the target branch to oauthjs:dev?

@Pritilender
Copy link
Contributor Author

@maxtruxa Yes, I'll do it some time tonight. Sorry, haven't seen the comment.

@Pritilender Pritilender changed the base branch from master to dev November 6, 2017 21:56
@Pritilender
Copy link
Contributor Author

@maxtruxa sorry I was quite busy that day when I've told that I'll update the branch... I updated the branch and reverted package-lock.json. I'll submit new PR now for package-lock.json.

Copy link
Contributor

@mjsalinger mjsalinger left a comment

Choose a reason for hiding this comment

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

LGTM

@mjsalinger mjsalinger dismissed maxtruxa’s stale review January 24, 2018 12:37

Reviewed based on new changes.

@mjsalinger mjsalinger merged commit af4a581 into oauthjs:dev Jan 24, 2018
@KingWu
Copy link

KingWu commented Jan 30, 2018

Will publish this fix?

@djoq
Copy link

djoq commented Feb 12, 2018

+1 on timeline for publishing.

@zo-chu
Copy link

zo-chu commented Feb 20, 2018

+1

@marceloch2
Copy link

marceloch2 commented Mar 3, 2018

Any news on this issue?

@Vergil0327
Copy link

Vergil0327 commented May 18, 2018

When will this patch be published ?

Still got this error on koa(v2.5.1) with oauth2-server(v3.0.0)

TypeError: options.headers.hasOwnProperty is not a function
at new Response (oauth2-server/lib/response.js:16:25)

@djoq
Copy link

djoq commented May 18, 2018

I have no clue, but I ran into the same issue and it is related to how the headers are constructed for the underlying oauth2-server package. I wrote a derivation based on this project without koa - https://github.com/djoq/o-provide for node.

For options in production, take a look at https://github.com/jazzband/django-oauth-toolkit.

@Vergil0327
Copy link

Vergil0327 commented May 21, 2018

The error will be fixed when change four lines of code like below.
92f0a0b
68ef835

But this fixed patch is still not published.
Any news about publish timeline?

@Pritilender
Copy link
Contributor Author

@Vergil0327 you can reference the dev branch in your package.json until someone from the maintainers publish it to npm.

@t1bb4r
Copy link

t1bb4r commented Dec 5, 2018

A year later... Is it still suggested to use the dev branch? I see some v4 tags and I'm scared of breaking changes.

EDIT: Found my answer here (#496), sorry for the ping

@Pritilender
Copy link
Contributor Author

Pritilender commented Dec 5, 2018

@t1bb4r As I still actively use this module in my projects, I can say that it's not pushed under current release on npm. I have v3.0.1 as my dependency for a current project (Express based, so no problems like this) and it doesn't include the code I changed in this PR.
I am not sure what's going on with v4. But I think that this is not a braking change and that it could be published as some patch version for v3 (paging @maxtruxa @mjsalinger).

Edit: It looks like it's on v3.1.0 branch.

@KingWu
Copy link

KingWu commented Feb 12, 2019

@djoq Still wait for publishing

@leonaves
Copy link

+1 pls publish

@djoq
Copy link

djoq commented Feb 13, 2019

@KingWu see #430 (comment)

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

Successfully merging this pull request may close these issues.

10 participants