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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add OperationHookContext interface #1820

Merged
merged 1 commit into from Feb 28, 2020

Conversation

hacksparrow
Copy link
Contributor

@hacksparrow hacksparrow commented Feb 24, 2020

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
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Commit messages are following our guidelines

@hacksparrow hacksparrow changed the title WIP chore: typings for supporting operation hooks in LB4 chore: typings for supporting operation hooks in LB4 Feb 25, 2020
@hacksparrow hacksparrow marked this pull request as ready for review February 25, 2020 12:31
Copy link
Member

@bajtos bajtos left a 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/persisted-model.d.ts Outdated Show resolved Hide resolved
types/persisted-model.d.ts Outdated Show resolved Hide resolved
types/observer-mixin.d.ts Show resolved Hide resolved
@@ -5,7 +5,11 @@

import {Callback, PromiseOrVoid} from './common';

export type Listener = (ctx: object, next: (err?: any) => void) => void;
export interface OperationHookContext {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@dhmlau dhmlau added this to the Feb 2020 milestone Feb 26, 2020
types/observer-mixin.d.ts Show resolved Hide resolved
types/persisted-model.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@bajtos bajtos left a 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:

https://github.com/strongloop/loopback-datasource-juggler/blob/36303b3b63932478cb429b5c5af6fec7f09b602c/lib/model.js#L868-L869

I think the missing piece is how to tell that information to TypeScript. Ideally, we want to say:

  • if a class T extends Model, then it will include ObserverMixin<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 implements ObserverMixin<Model>
  • PersistedModel implements ObserverMixin<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.

@bajtos
Copy link
Member

bajtos commented Feb 28, 2020

Regarding the commit message:

chore: typings for supporting operation hooks in LB4

Commit type chore is used for changes that are not affecting users. The changelog generator we use in loopback-next is excluding such commits from the log.

Since you are improving type definitions, you should use feat type.

@hacksparrow
Copy link
Contributor Author

Will edit in the final commit message after squashing the commits.

Added OperationHookContext interface.

Signed-off-by: Hage Yaapa <hage.yaapa@in.ibm.com>
@hacksparrow hacksparrow changed the title chore: typings for supporting operation hooks in LB4 feat: add OperationHookContext interface Feb 28, 2020
@hacksparrow hacksparrow merged commit dcfda58 into master Feb 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the feat/lb4-operation-hooks branch February 28, 2020 16:17
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.

None yet

5 participants