feathersjs / hooks

Async middleware for JavaScript and TypeScript
MIT License
157 stars 12 forks source link

fix(hooks): Allow to set context.result to undefined #70

Closed daffl closed 3 years ago

daffl commented 3 years ago

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 https://github.com/feathersjs/hooks/issues/69

vonagam commented 3 years ago

Curious, first time seeing:

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

Used instead of classic:

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

Is there any difference?

daffl commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

vonagam commented 3 years ago

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 commented 3 years ago

:shipit: