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(hooks): Allow to set context.result to undefined #70

Merged
merged 3 commits into from Feb 8, 2021

Conversation

daffl
Copy link
Member

@daffl daffl commented Feb 3, 2021

This pull request allows to set context.result to undefined in a before hook as suggested by @vonagam. It should be fully backwards compatible.

Closes #69

@daffl daffl mentioned this pull request Feb 3, 2021
@vonagam
Copy link
Member

vonagam commented Feb 3, 2021

Curious, first time seeing:

Object.getOwnPropertyDescriptor(context, 'result') === undefined

Used instead of classic:

Object.prototype.hasOwnProperty.call(context, 'result')

Is there any difference?

@daffl
Copy link
Member Author

daffl commented Feb 3, 2021

I believe it ends up being the same thing but Object.getOwnPropertyDescriptor is a more "recent" (ES5) spec and only applies to the actual object (instead the entire prototype chain) which I think is the right approach here since you can only set context.result on the actual object.

@vonagam
Copy link
Member

vonagam commented Feb 3, 2021

instead the entire prototype chain

Object.prototype.hasOwnProperty does the same, since otherwise it is not Own.

Just wanted to check, maybe there were some specific use case for which getOwnPropertyDescriptor were used here.

@daffl
Copy link
Member Author

daffl commented Feb 3, 2021

Just tested with hasOwnProperty and it also works fine. Updated the PR to use that since it's probably faster than getting the descriptor object.

@daffl
Copy link
Member Author

daffl commented Feb 4, 2021

If there are no objections, I can roll it out in a new release.

@vonagam
Copy link
Member

vonagam commented Feb 4, 2021

No real objections from me.

Only small note - people do call Object.prototype.hasOwnProperty, not Object.hasOwnProperty. (But since it resolves to the same thing in the end - because Object is an object instance too after all - it does not matter much.)

@bertho-zero
Copy link
Collaborator

:shipit:

@daffl daffl changed the title feat: Allow to set context.result to undefined fix(hooks): Allow to set context.result to undefined Feb 8, 2021
@daffl daffl merged commit 7b5807f into master Feb 8, 2021
@daffl daffl deleted the context-undefined branch February 8, 2021 21:33
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.

Result and undefined
3 participants