feat(cli): add `lb4 interceptor` to generate interceptors and a booter to load interceptors #2907

raymondfeng · 2019-05-15T23:27:45Z

Add lb4 interceptor command to generate interceptors.
Add interceptor booter to load interceptors from src/interceptors.

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 👈

bajtos

Nice addition.

I see two problematic parts:

I consider anonymous functions in Providers as a bad practice, let's use named class methods instead.
I'd like us to consider non-global interceptors too and find a design that can accommodate both global and non-global interceptors. Ideally, I'd like the CLI prompt to ask the developer whether they want to register the interceptor-being-created as a global one.

docs/site/Global-interceptor-generator.md

packages/boot/src/__tests__/fixtures/interceptor.artifact.ts

packages/boot/src/__tests__/integration/interceptor.booter.integration.ts

packages/boot/src/booters/interceptor.booter.ts

bajtos · 2019-05-24T06:38:55Z

packages/cli/generators/interceptor/index.js

+        name: 'group',
+        // capitalization
+        message: utils.toClassName(this.artifactInfo.type) + ' group:',
+        default: '',


How can the person running lb4 interceptor find out what are the valid group names?

What happens when they enter a group name that's not recognized by the app?

Based on our design, unknown group is sorted by alphabetical order before the known groups.

Based on our design, unknown group is sorted by alphabetical order before the known groups.

That answered my second question 👍

The first one is still not answered though:

How can the person running lb4 interceptor find out what are the valid group names?

Since the valid group names are configured by a binding, I'm not sure how we can check. Maybe we can improve the prompt to explain so.

Maybe we can improve the prompt to explain so.

Sounds good.

Ideally, I'd like the prompt to tell me:

What are the usual (default) group names

Where can I find the actual list of group names as recognized by my application

packages/cli/generators/interceptor/templates/interceptor-template.ts.ejs

bajtos · 2019-05-24T06:43:53Z

packages/context/src/interceptor.ts

-    binding.tag(ContextTags.GLOBAL_INTERCEPTOR);
+    binding
+      .tag(ContextTags.GLOBAL_INTERCEPTOR)
+      .tag({[ContextTags.NAMESPACE]: GLOBAL_INTERCEPTOR_NAMESPACE});


I am confused. Why do we need both GLOBAL_INTERCEPTOR tag and GLOBAL_INTERCEPTOR_NAMESPACE? I mean why is it not enough to use the tag only?

Is it a good idea to use the same namespace for all interceptors, regardless of which component/extension is contributing them? Wouldn't it better to use namespaces similarly to how Java treat packages, i.e. interceptors contributed by @loopback/authorization package should use @loopback/authorization namespace.

Let's discuss.

Only GLOBAL_INTERCEPTOR is required. GLOBAL_INTERCEPTOR_NAMESPACE is to make the binding key more readable.

Please add a code comment to capture this information.

Also, what is your opinion about using the same namespace for all interceptors vs. using a namespace based on the component/extension contributing the interceptor?

Personally, I prefer to use globalInterceptors.*.

raymondfeng · 2019-05-24T17:44:32Z

I consider anonymous functions in Providers as a bad practice, let's use named class methods instead.
I'd like us to consider non-global interceptors too and find a design that can accommodate both global and non-global interceptors. Ideally, I'd like the CLI prompt to ask the developer whether they want to register the interceptor-being-created as a global one.

Both are addressed.

bajtos

Looks mostly good at high level, let's do one more iteration to improve details and UX.

docs/site/Interceptor-generator.md

bajtos · 2019-06-04T11:51:16Z

docs/site/Interceptor-generator.md

+command. If provided, the tool will use that as the default when it prompts for
+the name.
+
+`--global` - Optional flag to indicate a global interceptor (default to `true`)


If the flag is true by default, what's the syntax for turning it off? Is it --no-global, --global=false or something else? Please explain that in the docs.

docs/site/Interceptor-generator.md

bajtos · 2019-06-04T11:53:33Z

docs/site/Interceptor-generator.md

+  */
+
+  value() {
+    const interceptor: Interceptor = async (invocationCtx, next) => {


Please rework the interceptor into a named method, i.e. TestInterceptor.prototype.intercept.

Suggested change

const interceptor: Interceptor = async (invocationCtx, next) => {

return this.intercept.bind(this);

Can you remind me why you are against using an arrow function inside value()? Is it about the stack trace? In my test, the stack trace shows interceptor (name of the const) as the function name, which is good to me.

Yes, it's mostly about the stack trace. IMO, TestInterceptor.intercept is most helpful.

interceptor is not entirely bad, but I see few problems:

It's not clear which interceptor is involved, one has to look to the filename to find that. (This works only when there is a single interceptor exported by that source file.)

It's not clear why we are storing the interceptor in a local variable. Personally, I would simplify the code as follows, with unintended consequence on stack traces:

value() { return async (invocationCtx, next) => { // ... }; }

More subjectively, this is also about Single Responsibility Principle - the value function should be concerned with obtaining the interceptor function only, the actual interception should be implemented elsewhere.

docs/site/Interceptor-generator.md

packages/boot/src/__tests__/fixtures/interceptor.artifact.ts

packages/boot/src/__tests__/fixtures/non-global-interceptor.artifact.ts

packages/cli/generators/interceptor/templates/interceptor-template.ts.ejs

packages/cli/test/integration/generators/interceptor.integration.js

bajtos

LGTM.

Please get at least one more approval from @strongloop/loopback-maintainers before landing.

bajtos · 2019-06-06T08:51:50Z

packages/context/src/interceptor.ts

    if (group) binding.tag({[ContextTags.GLOBAL_INTERCEPTOR_GROUP]: group});
  };
 }

+/**
+ * `@globalInterceptor` decorator to mark a class a global interceptor


Missing "as"? English is not my mother tongue, I may be wrong.

Suggested change

* `@globalInterceptor` decorator to mark a class a global interceptor

* `@globalInterceptor` decorator to mark a class as a global interceptor

bajtos · 2019-06-06T10:31:37Z

@raymondfeng I am not sure if this comment has been addressed yet: #2907 (comment)

Maybe we can improve the prompt to explain so.

Sounds good.

Ideally, I'd like the prompt to tell me:

What are the usual (default) group names

Where can I find the actual list of group names as recognized by my application

raymondfeng · 2019-06-06T14:23:33Z

@bajtos Did you notice that we now have the following prompt to address #2907 (comment)?

? Interceptor name: test
? Is it a global interceptor? Yes
? Global interceptors are sorted by the order of an array of group names bound t
o ContextBindings.GLOBAL_INTERCEPTOR_ORDERED_GROUPS. See https://loopback.io/doc
/en/lb4/Interceptors.html#order-of-invocation-for-interceptors.
Group name for the global interceptor: ('')

docs/site/Interceptor-generator.md

raymondfeng requested a review from bajtos as a code owner May 15, 2019 23:27

raymondfeng force-pushed the cli-interceptor branch from 58bc610 to d6308c0 Compare May 15, 2019 23:29

raymondfeng changed the title ~~feat(cli): add lb4 interceptor command to generate global interceptors~~ feat(cli): add lb4 interceptor to generate global interceptors and a booter to load global interceptors May 15, 2019

raymondfeng force-pushed the cli-interceptor branch 4 times, most recently from 16bdb94 to e74c2f1 Compare May 21, 2019 20:25

raymondfeng force-pushed the cli-interceptor branch 3 times, most recently from 6280b9b to da707f3 Compare May 23, 2019 17:01

bajtos suggested changes May 24, 2019

View reviewed changes

raymondfeng force-pushed the cli-interceptor branch 3 times, most recently from 8cf4fb0 to 8ec48b8 Compare May 24, 2019 17:43

raymondfeng requested a review from bajtos May 24, 2019 18:04

raymondfeng changed the title ~~feat(cli): add lb4 interceptor to generate global interceptors and a booter to load global interceptors~~ feat(cli): add lb4 interceptor to generate interceptors and a booter to load interceptors May 24, 2019

raymondfeng force-pushed the cli-interceptor branch 4 times, most recently from 7bb2c8a to 5fb119b Compare May 31, 2019 21:17

dhmlau added this to the June 2019 milestone milestone May 31, 2019

raymondfeng force-pushed the cli-interceptor branch from 5fb119b to 4b3b1b4 Compare June 3, 2019 16:49

dhmlau mentioned this pull request Jun 3, 2019

Monthly Milestone - Jun 2019 🌞🌺⛳️ #3035

Closed

37 tasks

bajtos reviewed Jun 4, 2019

View reviewed changes

raymondfeng force-pushed the cli-interceptor branch 2 times, most recently from bb1c657 to 1b7fe3f Compare June 4, 2019 15:28

raymondfeng requested a review from bajtos June 4, 2019 15:33

raymondfeng force-pushed the cli-interceptor branch from 1b7fe3f to a35ca5d Compare June 6, 2019 05:17

bajtos approved these changes Jun 6, 2019

View reviewed changes

bajtos requested a review from a team June 6, 2019 08:54

bajtos added the feature label Jun 6, 2019

raymondfeng added 4 commits June 6, 2019 07:26

feat(context): add @globalInterceptor decorator

5c8fb71

chore: simplify value() for interceptor providers

b2488ca

feat(cli): add lb4 interceptor command to generate interceptors

a60bcb9

feat(boot): add a booter for interceptors

552f101

raymondfeng force-pushed the cli-interceptor branch from a35ca5d to 552f101 Compare June 6, 2019 14:26

emonddr suggested changes Jun 6, 2019

View reviewed changes

docs/site/Interceptor-generator.md Show resolved Hide resolved

hacksparrow self-requested a review June 6, 2019 14:50

hacksparrow approved these changes Jun 6, 2019

View reviewed changes

raymondfeng requested a review from emonddr June 6, 2019 14:52

raymondfeng merged commit 467d6ea into master Jun 6, 2019

delete-merged-branch bot deleted the cli-interceptor branch June 6, 2019 15:07

bajtos mentioned this pull request Jun 14, 2019

Register Express middleware per route, controller class or controller method #2035

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): add `lb4 interceptor` to generate interceptors and a booter to load interceptors #2907

feat(cli): add `lb4 interceptor` to generate interceptors and a booter to load interceptors #2907

raymondfeng commented May 15, 2019 •

edited

Loading

bajtos left a comment

bajtos May 24, 2019

raymondfeng May 24, 2019

bajtos May 30, 2019

raymondfeng May 30, 2019

bajtos Jun 4, 2019

bajtos May 24, 2019

raymondfeng May 24, 2019

bajtos Jun 4, 2019

raymondfeng Jun 4, 2019

raymondfeng commented May 24, 2019

bajtos left a comment

bajtos Jun 4, 2019

bajtos Jun 4, 2019

raymondfeng Jun 4, 2019

bajtos Jun 6, 2019

bajtos left a comment

bajtos Jun 6, 2019

bajtos commented Jun 6, 2019

raymondfeng commented Jun 6, 2019

	const interceptor: Interceptor = async (invocationCtx, next) => {
	return this.intercept.bind(this);

	* `@globalInterceptor` decorator to mark a class a global interceptor
	* `@globalInterceptor` decorator to mark a class as a global interceptor

feat(cli): add lb4 interceptor to generate interceptors and a booter to load interceptors #2907

feat(cli): add lb4 interceptor to generate interceptors and a booter to load interceptors #2907

Conversation

raymondfeng commented May 15, 2019 • edited Loading

Checklist

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

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

raymondfeng commented May 24, 2019

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

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Jun 6, 2019

raymondfeng commented Jun 6, 2019

feat(cli): add `lb4 interceptor` to generate interceptors and a booter to load interceptors #2907

feat(cli): add `lb4 interceptor` to generate interceptors and a booter to load interceptors #2907

raymondfeng commented May 15, 2019 •

edited

Loading