Result and undefined

[vonagam]

To check if result is set and if a function should be skipped an equality to undefined is used. But what if one wants to set result to undefined as a real return value?

Maybe "result" in ctx check should be used instead as a more strict rule? (And if you need to remove already set result from previous hooks, you'll do delete ctx.result.)

(Just a thought, nothing more.)

[daffl]

You should already be able to set it to null instead.

[vonagam]

null is not undefined - they are not strictly equal and typescript won't even allow to assign null there.

[daffl]

The typing should be updated to allow null then since it should be possible. undefined is however not supported as a service return or hook result value.

[vonagam]

But hooks are not only about services, aren't they?

Well if the official position is that undefined should not be supported as a possible return value of a function - then this can be closed.

[daffl]

That's fair. One option would be to add an UNDEFINED symbol that you can set the result to in https://github.com/feathersjs/hooks so it would be possible going forward.

[vonagam]

Yeah, thought about it (symbol). But it creates problems when you work with arbitrary functions and set their result to result property. So now you'll need to replace their undefined with UNDEFINED and if you access context.result you need to remember about UNDEFINED and so on... (and I'm sure nobody will remember)

[vonagam]

It creates problems when undefined can mean both things - actual return value and absence of a value.

In feathers ecosystem you control rules and you can ban undefined as a return value of services, solving the ambiguity.

But when it is about random functions (many of which will be void, so should return only undefined) - i don't think you can make it work without opening door for bugs...

I assume that you don't like the idea because of breaking nature, right? Or is there anything else against it?

[daffl]

You're right, this is a good suggestion and I don't think it would be a breaking change.

I just created a pull request in #70 that should allow to do what you are suggesting by checking if there is a property descriptor for context.result. That way if context.result is ever set to anything at all in the hook chain, the method call will be skipped.