-
Notifications
You must be signed in to change notification settings - Fork 363
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
feat: add OperationHookContext interface #1820
Conversation
5b69937
to
c098104
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start, I have few comments to discuss.
types/observer-mixin.d.ts
Outdated
@@ -5,7 +5,11 @@ | |||
|
|||
import {Callback, PromiseOrVoid} from './common'; | |||
|
|||
export type Listener = (ctx: object, next: (err?: any) => void) => void; | |||
export interface OperationHookContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have API docs, example, r comments on what this new interface is for? Same for the rest of changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
d40c0fe
to
273ce78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes LGTM as an incremental improvement.
However, I am not sure if they are actually addressing the needs of loopbackio/loopback-next#4730?
As I understand the big picture, the problem is not in the fact that we don't have typings for observe()
, the current typings should be good enough (even if not as flexible and easy to use as we would like to). What we are missing is the information that every PersistedModel
includes ObserverMixin
. In fact, Model
classes provide Observer functionality too:
I think the missing piece is how to tell that information to TypeScript. Ideally, we want to say:
- if a class
T
extendsModel
, then it will includeObserverMixin<T>
methods
I am not sure if TypeScript makes this possible. If not, then I am fine to use a slightly less useful variant:
Model
implementsObserverMixin<Model>
PersistedModel
implementsObserverMixin<PersistedModel>
(I consider this a slightly less useful because hooks will have access to PersistedModel APIs only, they won't be able to access static methods and properties added by the particular model being observed.)
Since our primary motivation is to allow LB3 developers to migrate their operation hooks to LB4, it may be best to go with the second option and defer better typings to #1822 again.
Regarding the commit message:
Commit type Since you are improving type definitions, you should use |
Will edit in the final commit message after squashing the commits. |
Added OperationHookContext interface. Signed-off-by: Hage Yaapa <hage.yaapa@in.ibm.com>
cca079f
to
d9abd3c
Compare
Added typings for supporting operation hooks in LB4. Part of addressing loopbackio/loopback-next#3952.
Signed-off-by: Hage Yaapa hage.yaapa@in.ibm.com
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machine