1.0

[ritch]

/to @bajtos
/cc @raymondfeng

Please review the following.

https://github.com/strongloop/loopback-phase/blob/1.0/example/middleware.js => example for our middleware use case.

https://github.com/strongloop/loopback-phase/blob/1.0/test/phase.test.js => the phase API / test

https://github.com/strongloop/loopback-phase/blob/1.0/test/phase.test.js => the phaseSet API / test

Also take a look at the example in the README.md. Everything else is copied from loopback-boot.

[bajtos]

This provides a more helpful error message when the test fails:

expect(this).to.have.property(foo, 'bar');

[bajtos]

Can we use a different name than phase.register()? The name does not make it clear what is being registered where.

There is also inconsistency in phase.register() vs. phase.after(). The former is a verb, the latter is not.

If there is phase.after, should there be also phase.before? Is it a good idea to split each phase into sub-phases like this? Perhaps we should tell people to create a new phase instead of using phase.after.

What is the relation between phase.next and the order in the PhaseSet?

[ritch]

Can we use a different name than phase.register()? The name does not make it clear what is being registered where.

This name was original decided on in the overall component design...? I'd like to move forward since we already agreed on it there. I'm not really a fan of register as the name... but I don't really care what color we paint this bike shed.

If there is phase.after, should there be also phase.before? Is it a good idea to split each phase into sub-phases like this? Perhaps we should tell people to create a new phase instead of using phase.after.

I haven't had the need for it in my examples. I think its nice to allow anonymous handlers without having to create a phase. This is eventually what will allow people to easily hook into phases. Eg. you need something that is created in the foo phase you would write this:

app.phase('foo').after(useSomethingDefinedInFoo);

[ritch]

There is also inconsistency in phase.register() vs. phase.after(). The former is a verb, the latter is not.

These are shorthand for registerHandler and registerAfterHandler... Does that clarify at all? If you have single word names for these functions that are clearer LMK. Otherwise we can just make the names more explicit.

[bajtos]

Can we use a different name than phase.register()? The name does not make it clear what is being registered where.

This name was original decided on in the overall component design...?

IIRC, the decision was to come up with a better name when we get to review the code, so that the initial discussion can focus on the big picture.

I'd like to move forward since we already agreed on it there. I'm not really a fan of register as the name... but I don't really care what color we paint this bike shed.

I really don't want to waste my time on support explaining LoopBack users what is the intent of the register function. This is the new public API we will have to support for long time, it will be difficult to change it later. Therefore I want us to find a better name now.

If there is phase.after, should there be also phase.before? Is it a good idea to split each phase into sub-phases like this? Perhaps we should tell people to create a new phase instead of using phase.after.

I haven't had the need for it in my examples.

Fair enough, we can always add phase.before later.

I think its nice to allow anonymous handlers without having to create a phase. This is eventually what will allow people to easily hook into phases. Eg. you need something that is created in the foo phase you would write this:

app.phase('foo').after(useSomethingDefinedInFoo);

We are creating several ways how to order the execution of phase functions:

• app.phase('foo').after(fn)
• app.phase('foo').next(/* a phase calling the fn */)
• add "a phase calling the fn" into the set of app phases

My concerns:

• it may become difficult to reason about the code (i.e. understand the exact order in which functions are executed)
• it is not clear where to use which approach

BTW is after run before or after next?

There is also inconsistency in phase.register() vs. phase.after(). The former is a verb, the latter is not.

These are shorthand for registerHandler and registerAfterHandler... Does that clarify at all? If you have single word names for these functions that are clearer LMK. Otherwise we can just make the names more explicit.

Brainstorming few alternatives:

// before, on, after - `on` can be confused with EventEmitter :(app.phase('preprocess').on(function(){/*...*/});// beforeRun, onRun, afterRunapp.phase('preprocess').onRun(function(){/*...*/});// registerBeforeHandler, registerHandler, registerAfterHandlerapp.phase('preprocess').registerHandler(function(){/* ... */});// addBefore, addHandler, addAfterapp.phase('preprocess').addHandler(function(){/*...*/});

I agree this discussion should not change into bikeshedding. If we can't come up with anything better, I am fine with dropping register shorthand and using registerHandler.

[bajtos]

We may need to add an error handler (error logger?) function in the future. E.g. onError for onRun or registerErrorHandler for registerHandler. I can't come up with a good and simple name for the other two options (before/on/after; addBefore/addHandler/addAfter), which may be an indicator that they are not flexible enough and should be rejected.

[ritch]

We may need to add an error handler (error logger?) function in the future. E.g. onError for onRun or registerErrorHandler for registerHandler. I can't come up with a good and simple name for the other two options (before/on/after; addBefore/addHandler/addAfter), which may be an indicator that they are not flexible enough and should be rejected.

What is wrong with the launch() callback? I guess it won't be available to user code... An error event makes sense for something like this...

I think we should add this in another PR.

[ritch]

We are creating several ways how to order the execution of phase functions:

The only way you can add handlers is with the following methods:

• before()
• after()
• register()

next is a property (not a method) that is used to continue on to the next phase. It is documented... but maybe we should make it private...?

[ritch]

I agree this discussion should not change into bikeshedding. If we can't come up with anything better, I am fine with dropping register shorthand and using registerHandler.

What about:

phase.addHandler(function(cb){// ...});

[ritch]

Fair enough, we can always add phase.before later.

This ended up being trivial to add.

[ritch]

@kraman had a good idea for naming:

app.phase('preproccess').addTask(function(cb){// ...});app.phase('preprocess').addPreLaunchTask(function(){// ...});app.phase('preprocess').addPostLaunchTask(function(){// ...});

[ritch]

@bajtos / other reviewers - please take a look at the docs as well:

git checkout 1.0
sdocs -p

[raymondfeng]

A few comments:

1. phaseSet.add() and phaseSet.addInOrder() should be consolidated into phaseSet.add() which can take a string or string[].
2. phaseSet.findById() should be renamed to phaseSet.phase().
3. phaseSet.findOrAdd() can be consolidated into phaseSet.phase(name, forceCreate)
4. Phase apis

app.phase('preproccess').use(function(cb){// ...});app.phase('preprocess').pre(function(){// ...});app.phase('preprocess').post(function(){// ...});

[ritch]

phaseSet.add() and phaseSet.addInOrder() should be consolidated into phaseSet.add() which can take a string or string[].

I thought about this. I prefer the redundancy and the clarity in the documentation. Specifying that addInOrder is a special method that orders phases is otherwise difficult to document.

[ritch]

phaseSet.findById() should be renamed to phaseSet.phase().

This doesn't really make sense in the context of a Set. I think the method in loopback will be app.phase() though.

[altsang]

+1 addHandler(), is there a removeHandler() as well?

strange to me as well to have after() without before() and if next is "special" - +1 for making it private

I do agree with @bajtos to a certain degree that if this is a public API that we should try to get to a good level of consistency upfront but not end up in the bikeshed

[ritch]

phaseSet.findOrAdd() can be consolidated into phaseSet.phase(name, forceCreate)

I've learned my lesson in strong-remoting accepting booleans like this. I prefer the clarity in the method name.

[ritch]

Phase apis

This is pretty good. +1 - @bajtos@kraman thoughts?

[ritch]

@altsang

I do agree with @bajtos to a certain degree that if this is a public API that we should try to get to a good level of consistency upfront but not end up in the bikeshed

Yep we have before() and after()... I'll add removeHandler() as well.

[bajtos]

@raymondfengphaseSet.findOrAdd() can be consolidated into phaseSet.phase(name, forceCreate)
@ritch I've learned my lesson in strong-remoting accepting booleans like this. I prefer the clarity in the method name.

-100 for boolean parameters, it's a strong code smell.

+100 for using different a method name documenting the intent of the method

---

As for naming, any of these work fine for me:

• addPreLaunchTask, addTask, addPostLaunchTask + removeTask, removePreLaunchTask, removePostLaunchTask.
• pre, use, post + removeUse (??), removePre, removePost (or perhaps phase.use.remove, phase.pre.remove?)

Few more alternatives:

• addPreProcessor, addProcessor, addPostProcessor (+ remove*Processor)
• addPreHandler, addHandler, addPostHandler (+remove*Handler)

[bajtos]

next is a property (not a method) that is used to continue on to the next phase. It is documented... but maybe we should make it private...?

Consider this example:

varphaseSet=newPhaseSet();phaseSet.addInOrder(['one','two','three']);phaseSet.phase('one').next=newPhase();

My expectations:

• the phases are executed in this order: one, (anonymous), two, three.
• the after fn should be called after one finished but before (anonymous) is run

Which opens another point - your test suite is missing tests verifying the order in which the phases and their handlers are executed.

[ritch]

Which opens another point - your test suite is missing tests verifying the order in which the phases and their handlers are executed.

Take a look at this...
https://github.com/strongloop/loopback-phase/blob/1.0/test/phase.test.js#L65

I can add a before handler to ensure that is always executed first.

[ritch]

My expectations:

• the phases are executed in this order: one, (anonymous), two, three.
• the after fn should be called after one finished but before (anonymous) is run

This is the behavior. If you have suggestions on how to clarify on this docs LMK.

[ritch]

Just had a discussion with @raymondfeng and @bajtos. We decided to go with:

before()after()use()

[bajtos]

Please add some tests verifying phase ordering. E.g. that foo will be executed before bar.

[bajtos]

OK, I see the ordering is covered by PhaseList phases.toArray() should return the list of phases as an array.

[raymondfeng]

Each PhaseList should have a name.

[bajtos]

A subjective opinion: I find it better to add .test postfix to test filenames, it makes it easier to quickly find them via filename using WebStorm's "Find file" or ViM's CtrlP plugin. test/phase-list.test.js instead of test/phase-list.js.

Whatever option you choose, please use it consistently. ATM there is test/phase.test.js and test/phase-list.js.

[ritch]

A subjective opinion: I find it better to add .test postfix to test filenames, it makes it easier to quickly find them via filename using WebStorm's "Find file" or ViM's CtrlP plugin. test/phase-list.test.js instead of test/phase-list.js.

Fixed

[bajtos]

@ritch1.0.0 was released to npmjs, but there is no git tag. Did you forget to push the tag to github?