feat: support operation hooks #4730

hacksparrow · 2020-02-24T14:43:55Z

Added support for operation hooks. Addresses #3952.

Signed-off-by: Hage Yaapa [email protected]

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
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

raymondfeng · 2020-02-24T18:01:05Z

docs/site/migration/models/operation-hooks.md

+
+  definePersistedModel(entityClass: typeof Product) {
+    const model = super.definePersistedModel(entityClass);
+    model.observe('before save', async ctx => {


We can access the model as this.modelClass. Why not leveraging that to set up observers instead of overriding definedPersistedModel?

Initially we were considering having an observe method on DefaultCrudRepository, which would call this.modelClass.observe. After some brainstorming, @bajtos suggested the possible problems with observe is not worth it. Eg: calling observe() multiple times and how to handle it. He had suggested another approach but it wasn't very DX-friendly, so we settled on the current one.

"You want to directly access the model? Then you will have to override the definePersistedModel method."

@raymondfeng please check the description of the user story and the discussion in that GH issue for more context.

@hacksparrow please update PR description with a reference to the GH issue this is related to.

@hacksparrow We do support multiple observers for a given model class/operation - see https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/observer.js#L51

Yes, that's understood. We were wondering how should LB4 respond when an operation hook is installed multiple times: a. Throw an error? b. Accept silently - how does the user know what happened?

Also, it is not impossible to call the observe method before the modelClass is defined. What happens in that case?

Since this is a temporary workaround, we didn't want to invest too much time handling various possible scenarios, edge cases etc. We can do that when we investigate the use of interceptor for the "real" implementation.

hacksparrow · 2020-02-25T12:37:06Z

Needs loopbackio/loopback-datasource-juggler#1820 to land for tests to pass.

dhmlau · 2020-02-26T14:33:46Z

docs/site/migration/models/operation-hooks.md

-following GitHub issue:
-[loopback-next#3952](https://github.com/strongloop/loopback-next/issues/3952)
-" %}
+Operation hooks are not supported in LoopBack 4 yet. It will be, eventually; you


Are we really going to have operation hooks in LB4? Maybe reword it as:
See the [Operation hooks for models/repositories spike](https://github.com/strongloop/loopback-next/issues/1919) for the progress

Updated as per suggestion.

jannyHou

LGTM 👍

jannyHou · 2020-02-26T15:09:25Z

docs/site/migration/models/operation-hooks.md

+Make sure to return the model instance from the derived class.
+
+Here is an example of a repository implementing `definePersistedModel` and
+applying an operation hook on a model instance:


nitpick: I remember the observe method is called by a model class, not instance, not sure is if it's still the case in the legacy juggler bridge.

Thanks! Corrected.

agnes512

Not familiar with the area but it looks reasonable to me 👍

agnes512 · 2020-02-26T16:16:34Z

packages/repository/src/__tests__/acceptance/operation-hooks.acceptance.ts

@@ -0,0 +1,56 @@
+// Copyright IBM Corp. 2019. All Rights Reserved.


// Copyright IBM Corp. 2020

nabdelgadir

I'm not too familiar with this area but the changes look reasonable to me

bajtos

The changes look mostly good, kudos for adding a unit test to verify the solution we are offering in the docs 👏

Needs loopbackio/loopback-datasource-juggler#1820 to land for tests to pass.

I think it's not enough to land the PR, we need the change to be published to npm too.

packages/repository/src/__tests__/acceptance/operation-hooks.acceptance.ts

docs/site/migration/models/operation-hooks.md

bajtos

Almost there 👏

I have few minor comments to address, PTAL.

bajtos · 2020-02-28T08:40:15Z

docs/site/migration/models/operation-hooks.md

+operation hooks a process that is possible only after the model class is
+attached to the repository and accidental registration of the same operation
+hook multiple times becomes obvious. With an API which directly exposes the
+`observe` method of the model class, this would not have beeb possible.


Suggested change

`observe` method of the model class, this would not have beeb possible.

`observe` method of the model class, this would not have been possible.

bajtos · 2020-02-28T09:23:08Z

packages/repository/src/repositories/legacy-juggler-bridge.ts

@@ -5,7 +5,7 @@

 import {Getter} from '@loopback/context';
 import assert from 'assert';
-import legacy from 'loopback-datasource-juggler';
+import legacy, {ObserverMixin} from 'loopback-datasource-juggler';


I think we are intentionally keeping juggler types very separated from LB4 types. Take a look at lines 41-53, where we are re-exporting a subset of legacy types in juggler namespace.

Please make the following changes:

Revert this line back.

Re-export legacy.ObserverMixin in juggler namespace

Modify all places below using ObserverMixin to use juggler.Mixin

Please add a comment to all those modified places to mention that it's a temporary workaround and add a link to the GitHub issue tracking the follow-up task of improving juggler typings to correctly include ObserverMixin in PersistedModelClass API.

packages/repository/src/repositories/legacy-juggler-bridge.ts

bajtos · 2020-02-28T09:24:51Z

packages/repository/src/repositories/legacy-juggler-bridge.ts

+    return this.definePersistedModel(entityClass);
+  }
+
+  // Create an internal legacy Model attached to the datasource


Please replace this comment with a proper tsdoc-style one, explain LB4 app developers that they can override this method in their custom repository classes to tweak the legacy Model (e.g. register operation hooks).

bajtos

LGTM. Please fix the tsdoc per my comment below and make sure all CI checks are green (I think there are linting problems in the codebase and commit messages.)

packages/repository/src/repositories/legacy-juggler-bridge.ts

dhmlau

@hacksparrow, I was trying to fix the code linting error in this PR, but didn't go further. I'm not sure if it's because we didn't release the change in loopbackio/loopback-datasource-juggler#1820. So, I left some comments.

dhmlau · 2020-02-28T20:41:08Z

packages/repository/src/__tests__/acceptance/operation-hooks.acceptance.ts

+  const expectedArray = [beforeSave, afterSave];
+
+  it('supports operation hooks', async () => {
+    const p = await repo.create({slug: 'pencil'});


code linter complaining about p is not being used.
Should change this to:

await repo.create({slug: 'pencil'});

dhmlau · 2020-02-28T20:42:22Z

packages/repository/src/__tests__/acceptance/operation-hooks.acceptance.ts

+
+    definePersistedModel(entityClass: typeof Product) {
+      const modelClass = super.definePersistedModel(entityClass);
+      modelClass.observe(beforeSave, async ctx => {


code linter is complaining about Promise returned in function argument where a void return was expected. It might be because we're waiting for your changes in juggler to be released: loopbackio/loopback-datasource-juggler#1820.

dhmlau · 2020-02-28T20:42:28Z

packages/repository/src/__tests__/acceptance/operation-hooks.acceptance.ts

+        this.hooksCalled.push(beforeSave);
+      });
+
+      modelClass.observe(afterSave, async ctx => {


code linter is complaining about Promise returned in function argument where a void return was expected. It might be because we're waiting for your changes in juggler to be released: loopbackio/loopback-datasource-juggler#1820.

hacksparrow · 2020-03-05T08:58:09Z

@slnode test please

hacksparrow · 2020-03-05T09:35:18Z

@slnode test please

bajtos

Nice! I love how much simpler is the new supporting implementation in @loopback/repository 👏

I have a tiny comment regarding the migration guide, feel free to ignore it.

bajtos · 2020-03-05T15:13:28Z

docs/site/migration/models/operation-hooks.md

+
+Although possible, we are not providing an API which directly exposes the
+`observe` method of the model class. The current API makes the registration of
+operation hooks a process that is possible only after the model class is


Suggested change

operation hooks a process that is possible only after the model class is

operation hooks a process that is possible only at the time when the model class is

Add support for operation hooks. Signed-off-by: Hage Yaapa <[email protected]>

raymondfeng · 2020-03-05T15:52:43Z

packages/repository/src/repositories/legacy-juggler-bridge.ts

@@ -148,6 +148,21 @@ export class DefaultCrudRepository<
      return model as typeof juggler.PersistedModel;
    }

+    return this.definePersistedModel(entityClass);


Why don’t we emit an event when definePersistedModel is done so that listeners can be registered to set up operation hooks? Overriding a method is always more advanced for a lot of developers.

For the record, we briefly discussed if it's feasible to use events to set up operation hooks and decided to defer that.

hacksparrow changed the title ~~WIP: feat: support operation hooks~~ WIP feat: support operation hooks Feb 24, 2020

raymondfeng reviewed Feb 24, 2020

View reviewed changes

hacksparrow force-pushed the feat/operation-hooks branch from 07f33c7 to 4305959 Compare February 25, 2020 10:49

hacksparrow changed the title ~~WIP feat: support operation hooks~~ feat: support operation hooks Feb 25, 2020

hacksparrow marked this pull request as ready for review February 25, 2020 12:31

hacksparrow requested review from agnes512, deepakrkris and nabdelgadir as code owners February 25, 2020 12:31

emonddr approved these changes Feb 25, 2020

View reviewed changes

dhmlau reviewed Feb 26, 2020

View reviewed changes

jannyHou approved these changes Feb 26, 2020

View reviewed changes

agnes512 reviewed Feb 26, 2020

View reviewed changes

dhmlau added this to the Feb 2020 milestone Feb 26, 2020

nabdelgadir approved these changes Feb 27, 2020

View reviewed changes

bajtos reviewed Feb 27, 2020

View reviewed changes

packages/repository/src/__tests__/acceptance/operation-hooks.acceptance.ts Outdated Show resolved Hide resolved

docs/site/migration/models/operation-hooks.md Show resolved Hide resolved

hacksparrow force-pushed the feat/operation-hooks branch from 6bff685 to 471ad03 Compare February 27, 2020 14:02

bajtos mentioned this pull request Feb 28, 2020

Add ObserverMixin to ModelBase typings loopbackio/loopback-datasource-juggler#1823

Merged

7 tasks

bajtos reviewed Feb 28, 2020

View reviewed changes

bajtos approved these changes Feb 28, 2020

View reviewed changes

packages/repository/src/repositories/legacy-juggler-bridge.ts Outdated Show resolved Hide resolved

dhmlau reviewed Feb 28, 2020

View reviewed changes

hacksparrow mentioned this pull request Mar 3, 2020

chore: improve Listener signature loopbackio/loopback-datasource-juggler#1826

Merged

4 tasks

dhmlau modified the milestones: Feb 2020, Mar 2020 Mar 3, 2020

hacksparrow force-pushed the feat/operation-hooks branch 3 times, most recently from 6ac39b1 to eaf2579 Compare March 4, 2020 14:10

hacksparrow force-pushed the feat/operation-hooks branch from eaf2579 to 2222b54 Compare March 5, 2020 09:37

hacksparrow force-pushed the feat/operation-hooks branch 3 times, most recently from 7444da9 to ee077b8 Compare March 5, 2020 13:53

bajtos approved these changes Mar 5, 2020

View reviewed changes

feat: support operation hooks

e30f896

Add support for operation hooks. Signed-off-by: Hage Yaapa <[email protected]>

hacksparrow force-pushed the feat/operation-hooks branch from ee077b8 to e30f896 Compare March 5, 2020 15:17

dhmlau self-requested a review March 5, 2020 15:37

raymondfeng reviewed Mar 5, 2020

View reviewed changes

raymondfeng approved these changes Mar 5, 2020

View reviewed changes

hacksparrow merged commit 8701cce into master Mar 5, 2020

hacksparrow deleted the feat/operation-hooks branch March 5, 2020 16:19

hacksparrow mentioned this pull request Mar 6, 2020

How to migrate CRUD Operation Hooks #3952

Closed

3 tasks

agnes512 removed this from the Mar 2020 milestone Mar 9, 2020

dhmlau added Docs Migration labels Mar 11, 2020

		@@ -0,0 +1,56 @@
		// Copyright IBM Corp. 2019. All Rights Reserved.

	`observe` method of the model class, this would not have beeb possible.
	`observe` method of the model class, this would not have been possible.

	operation hooks a process that is possible only after the model class is
	operation hooks a process that is possible only at the time when the model class is

feat: support operation hooks #4730

feat: support operation hooks #4730

Conversation

hacksparrow commented Feb 24, 2020 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnes512 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabdelgadir left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

dhmlau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow commented Mar 5, 2020

hacksparrow commented Mar 5, 2020

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow commented Feb 24, 2020 •

edited

Loading