[RFC] Port loopback-phase to TypeScript for LoopBack 4 #1671

raymondfeng · 2018-09-05T20:26:42Z

This PR ports loopback-phase to TypeScript with improvements:

Promise based handler
Allow the handler to control how to proceed if it's invoked as part of the invocation chain (supporting both express and koa styles)
Special error and final phase to mimic try - catch - finally

This package can be used as the foundation for the use cases such as:

Express middleware registration/execution by phase
Sequence of action for req/res processing
Sequence of boot/lifecycle steps

Checklist

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

bajtos

I am not sure if phases is the right design approach, but don't mind experimenting in this area.

I think this work is out of 4.0 GA scope, could you please confirm?

bajtos · 2018-09-06T09:35:16Z

packages/phase/README.md

+```ts
+async (ctx, chain) => {
+  const start = process.hrtime();
+  await chain!.next();


Please find a better type description for the callback function, one that does not require users to use ! operator. If the framework always provide chain value, then this fact should be encoded in the type signature of the callback.

bajtos · 2018-09-06T09:36:27Z

packages/phase/README.md

+async (ctx, chain) => {
+  if (ctx.req.url === '/status') {
+    ctx.response = {status: 'ok'};
+    chain!.return();


I find return as ambiguous. Can we use a name that better expresses the intent please? For example: abort, terminate or stop.

bajtos · 2018-09-06T09:40:43Z

packages/phase/README.md

+
+There is also a `failFast` option to control how to handle errors. If `failFast`
+is set to `false`, errors will be caught and set on the `ctx.error` without
+aborting the handler chain.


I find this rather dangerous. I expect that most people writing phase-handlers are not going to be aware of the failFast:false mode. As a result, failFast:false mode can execute handlers that are not prepared to handle the situation left after one of the previous handlers failed.

I think a better solution would be to enable failFast:false mode on a per-handler basis - i.e. allow each handler to signal whether it wants to be executed even if there was an error up in the chain.

What is the use case you are envisioning for failFast:false mode?

Can we perhaps leave this feature out of this initial pull request, to get it landed faster?

I introduce failFast:false mode mostly for error phase so that we can have a list of error handlers to mediate errors.

bajtos · 2018-09-06T09:41:53Z

packages/phase/README.md

+} catch (e) {
+  // await run handlers for errorPhase
+} finally {
+  // await run handlers for finalPhase


Are we enforcing the order of $error and $final phase? Can users insert additional phases before, in-between or after these two built-in phases?

Yes, we enforce them as catch and finally. No additional phases can interfere with them.

bajtos · 2018-09-06T09:42:26Z

packages/phase/package.json

+{
+  "name": "@loopback/phase",
+  "version": "0.1.0",
+  "description": "LoopBack's API Explorer",


Please fix the package description.

bajtos · 2018-09-06T09:45:47Z

packages/phase/src/handler.ts

+  return async (ctx: Context) => {
+    if (options.parallel) {
+      debug('Executing handlers in parallel');
+      await Promise.all(handlers.map(h => h(ctx)));


I don't think it's a good idea to run all handlers in parallel.

What if we have 3 handlers registered and the handler no.2 calls ctx.return()? It would be surprised to learn that the 3rd handler was executed to completion too!

What if we have 2 handlers registered and the first handler throws an error? I would find it surprising that the second handler was still executed. Also, are we going to wait until that second handler finishes, or will we return the error immediately?

Also, what's the point of Phase#parallel option when we are always running all handlers in parallel?

Please add tests to capture the expected behavior and verify the implementation.

bajtos · 2018-09-06T09:47:23Z

packages/phase/src/phase-list.ts

+   * @returns phaseNames
+   */
+  getPhaseNames(): string[] {
+    return this.phases.map(p => p.id);


Should we include $error and $final in this list?

bajtos · 2018-09-06T09:47:41Z

packages/phase/src/phase-list.ts

+   * @param handler The handler function to register
+   *   with the given phase.
+   */
+  registerHandler(phaseName: string, handler: Handler) {


What's the API for registering handlers for $error and $final phases?

bajtos · 2018-09-06T09:50:15Z

packages/phase/test/unit/phase.unit.ts

+          .use(mockHandler('h1'))
+          .after(mockHandler('a1'))
+          .after(mockHandler('a2'))
+          .use(mockHandler('h2'));


Please use a more descriptive names, I find it difficult to follow these terse two-letter identifiers.

For example: before-handlerA-1, before-handlerA-2, handlerA, after-handlerA-1, etc. handlerB.

virkt25

Had a quick look. Didn't point out all the places the year / name needs to be updated. Had some questions to get a better understanding of this package. Will do another pass later to go over the tests.

virkt25 · 2018-09-11T23:48:49Z

packages/phase/src/handler.ts

@@ -0,0 +1,154 @@
+// Copyright IBM Corp. 2014. All Rights Reserved.


nitpick / reminder: copyright year and node module need to be updated.

virkt25 · 2018-09-11T23:50:18Z

packages/phase/src/handler.ts

+/**
+ * Context for the invocation of handlers by phase
+ */
+export interface Context {


As a general cleanup of this file I think it might be best to introduce a interfaces.ts or types.ts which defines all the interfaces since they are cluttering this file imo.

virkt25 · 2018-09-11T23:51:23Z

packages/phase/src/index.ts

@@ -0,0 +1,8 @@
+// Copyright IBM Corp. 2018. All Rights Reserved.
+// Node module: loopback-phase


reminder: update module name.

virkt25 · 2018-09-11T23:54:59Z

packages/phase/src/phase-list.ts

@@ -0,0 +1,320 @@
+// Copyright IBM Corp. 2018. All Rights Reserved.
+// Node module: loopback-phase


update module name

virkt25 · 2018-09-11T23:58:47Z

packages/phase/src/phase-list.ts

+   * @param phase The phase to be added
+   */
+  add(phase: string | Phase) {
+    return this.addAll(phase)[0];


Most implementations I've seen, the logic usually lives in the singular form of the function and the plural form is the one that calls the singular form as it loops over the entries. I'm curious to learn as to why this implementation is flipped in this particular case.

virkt25 · 2018-09-12T00:03:45Z

packages/phase/README.md

+
+### Handler
+
+A handler is a function that takes a `Context` object, an optional


Till I dug into the code, I found the usage of the work Context here confusing as I thought this was referring to @loopback/context.

bajtos · 2019-01-07T10:33:16Z

@raymondfeng what's the status of this pull request - is it still relevant?

stale · 2020-04-11T20:32:02Z

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

raymondfeng · 2020-06-01T17:34:38Z

Closing it to favor middleware chain.

raymondfeng requested a review from bajtos as a code owner September 5, 2018 20:26

raymondfeng force-pushed the phase branch 2 times, most recently from 72d8eb9 to 0d9b816 Compare September 5, 2018 20:49

bajtos suggested changes Sep 6, 2018

View reviewed changes

raymondfeng added the post-GA label Sep 6, 2018

raymondfeng force-pushed the phase branch 7 times, most recently from 6c84945 to 6c1be3a Compare September 7, 2018 16:14

virkt25 reviewed Sep 12, 2018

View reviewed changes

raymondfeng force-pushed the phase branch from 6c1be3a to 41ca204 Compare September 12, 2018 16:45

raymondfeng mentioned this pull request Sep 18, 2018

[WIP][RFC] Add local swagger ui for LoopBack 4 API explorer #1664

Closed

7 tasks

raymondfeng force-pushed the phase branch from 41ca204 to 6a35b24 Compare October 29, 2018 20:39

dhmlau removed the post-GA label Nov 2, 2018

raymondfeng force-pushed the phase branch from 6a35b24 to 0aeabe0 Compare November 8, 2018 21:15

raymondfeng mentioned this pull request Nov 16, 2018

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

Closed

raymondfeng mentioned this pull request Dec 3, 2018

feat(core): introduce basic life cycle support for LoopBack applications #1928

Merged

7 tasks

raymondfeng force-pushed the phase branch from 0aeabe0 to 4c91a4e Compare December 8, 2018 02:18

raymondfeng force-pushed the phase branch from 4c91a4e to df17089 Compare February 8, 2019 18:22

bajtos added the feature label Feb 12, 2019

raymondfeng force-pushed the phase branch from df17089 to 47ec8a7 Compare February 14, 2019 22:26

raymondfeng mentioned this pull request Mar 6, 2019

[RFC] Introduce express middleware extension point #2434

Closed

8 tasks

raymondfeng force-pushed the phase branch 3 times, most recently from 5e57668 to 7654914 Compare March 7, 2019 23:45

raymondfeng force-pushed the phase branch from 7654914 to 927f258 Compare April 4, 2019 16:22

raymondfeng added 2 commits April 17, 2019 13:15

feat(phase): add @loopback/phase package

97fd65b

feat(rest): use phase to manage express middleware

d79ef13

raymondfeng force-pushed the phase branch from 927f258 to d79ef13 Compare April 17, 2019 20:15

stale bot added the stale label Apr 11, 2020

raymondfeng closed this Jun 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Port loopback-phase to TypeScript for LoopBack 4 #1671

[RFC] Port loopback-phase to TypeScript for LoopBack 4 #1671

raymondfeng commented Sep 5, 2018 •

edited

Loading

bajtos left a comment

bajtos Sep 6, 2018

bajtos Sep 6, 2018

bajtos Sep 6, 2018

raymondfeng Sep 6, 2018

bajtos Sep 6, 2018

raymondfeng Sep 6, 2018

bajtos Sep 6, 2018

bajtos Sep 6, 2018

bajtos Sep 6, 2018

bajtos Sep 6, 2018

bajtos Sep 6, 2018

virkt25 left a comment

virkt25 Sep 11, 2018

virkt25 Sep 11, 2018

virkt25 Sep 11, 2018

virkt25 Sep 11, 2018

virkt25 Sep 11, 2018

virkt25 Sep 12, 2018

bajtos commented Jan 7, 2019

stale bot commented Apr 11, 2020

raymondfeng commented Jun 1, 2020

		@@ -0,0 +1,154 @@
		// Copyright IBM Corp. 2014. All Rights Reserved.

		@@ -0,0 +1,8 @@
		// Copyright IBM Corp. 2018. All Rights Reserved.
		// Node module: loopback-phase

		@@ -0,0 +1,320 @@
		// Copyright IBM Corp. 2018. All Rights Reserved.
		// Node module: loopback-phase


		### Handler

		A handler is a function that takes a `Context` object, an optional

[RFC] Port loopback-phase to TypeScript for LoopBack 4 #1671

[RFC] Port loopback-phase to TypeScript for LoopBack 4 #1671

Conversation

raymondfeng commented Sep 5, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virkt25 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

bajtos commented Jan 7, 2019

stale bot commented Apr 11, 2020

raymondfeng commented Jun 1, 2020

raymondfeng commented Sep 5, 2018 •

edited

Loading