Skip to content
This repository has been archived by the owner on Apr 20, 2018. It is now read-only.

[discussion] maybe change the name of the context parameter from 'hook' to 'context' or use 'this' in the future? #159

Closed
beeplin opened this issue May 5, 2017 · 10 comments

Comments

@beeplin
Copy link

beeplin commented May 5, 2017

Currently the hook function written as (hook) => {...; return hook; } is somehow confusing for many new users. The term hook has two meanings here: the function itself is called a hook or a hook function, but the context parameter passed into the hook function is also called hook.

In fact the parameter naming is arbitrary, and calling it hook is just a convention or a tradition of feathers which is initiated in the official docs. So I propose to (in the future) change the docs and rename it to something more clear, like context or so. So people use context.app, context.path etc. instead of hook.app, hook.path.

Another possible options is to use this instead of a parameter to pass the context object into the hook function. So people just use this.app or this.path ... I know this in hook function used to point to the current service object, so this might have some backward compatibility issues. But on the other hand, today we have the new syntax of arrow functions => which keepsthis and we don't need to manually bind this again and again, which makes the use of this in such situation quite concise and elegant and appealing.

@eddyystop
Copy link
Contributor

People seem to just not like using this. That is the main reason hook.service was introduced.

I've also run into (transpiling?) issues passing params with .call(). See feathersjs-ecosystem/feathers-hooks-common#140 (comment) and feathersjs-ecosystem/feathers-hooks-common#116 . Thew problem is either me or Babel.

Your comments about 'rationalizing' naming have always been spot on. However it may be wiser to not use this.

@beeplin
Copy link
Author

beeplin commented May 5, 2017

hmmm, I have never used hooks with babel... I agree this might be confusing too and sometimes problematic. How do you think of renaming it to context?

@marshallswain
Copy link
Member

marshallswain commented May 5, 2017

I like the separation of hook = function. context = hook object

@eddyystop
Copy link
Contributor

eddyystop commented May 5, 2017

context doesn't carry much meaning, so we can do better. Maybe something like request, even package suggests the thing gets passed along.

@beeplin
Copy link
Author

beeplin commented May 5, 2017

I have been considering request and response before, but I doubt request might be too narrow since we also use the term in after hooks and request.result doesn't make any sense. If we use response in after hooks instead, then response.data seems weird too. And they can also be confusing since express/feathers middleware use request/response for something different, and we often have hooks and middleware in the one project at the same time.

context Is the best I can think of so far. Many projects use context in similar circumstances such as vue, ajv and etc.

@daffl
Copy link
Member

daffl commented May 9, 2017

I do like context. The problem with using this is that it already is the service the hook is running on and you can't set this in ES6 arrow functions (which is one of the reasons we added the .service property). I think request is too confusing with the HTTP request/response terminology.

@ekryski
Copy link
Member

ekryski commented May 10, 2017

FWIW I 100% agree with @daffl's comment. I'd like to see context. Also, related feathersjs/feathers#562

@eddyystop
Copy link
Contributor

eddyystop commented May 10, 2017 via email

@beeplin
Copy link
Author

beeplin commented May 11, 2017

As a non English speaker I think state might be more ambiguous than context. Context has one and only one specific meaning which is easier to understand.

@eddyystop
Copy link
Contributor

The renaming has started with feathersjs-ecosystem/feathers-hooks-common#200 .

An issue has been created for the rest feathersjs-ecosystem/feathers-hooks-common#201 .

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

No branches or pull requests

5 participants