Decouple application config from features (e.g. components) #975

shimks · 2018-02-07T18:14:18Z

Tweaked ApplicationConfig so that it can no longer be used to set up components, controllers, or servers
- Components must now be registered through app.component(MyComponent)
- Controllers and servers as well through .controller() and .server()
~~Added in functions~~ Overloaded existing functions to take in multiple components/controllers/servers
- component([My, List, Of, Components])
- server([My, List, Of, Servers])
- controller([My, List, Of, Controllers)
Also tweaked RepositoryMixin so that repositories can no longer be set up through the constructor
- overloaded repository to also be able to take in an array of Repositoryies
- overloaded component to also be able to take in an array of components
First commit involves the code while the second commit involves READMEs
connected to Decouple wiring of implementation bits from application configuration #742

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Related API Documentation was updated
Affected artifact templates in packages/cli were updated
Affected example projects in packages/example-* were updated

kjdelisle

As long as we know it's okay to remove those deps, then I'm satisfied.

kjdelisle · 2018-02-07T18:31:14Z

packages/core/README.md

  rest: {
    port: 3000,
  },
  grpc: {
    port: 3001,
  },
 });
+app.components([RestComponent, GrpcComponent]);


As long as there's a comment here to make it clear (that's why those other lines were commented in that way), I'm cool with this.

kjdelisle · 2018-02-07T18:32:00Z

packages/core/package.json

@@ -21,9 +21,7 @@
  "copyright.owner": "IBM Corp.",
  "license": "MIT",
  "dependencies": {
-    "@loopback/context": "^4.0.0-alpha.31",
-    "lodash": "^4.17.4",


Was lodash supposed to be here? (I can't remember, but I think it was used somewhere at one point)

Why is it being removed now?

Those dependencies weren't being used at all. Not really a part of the task, but I dropped them anyway

b-admike · 2018-02-07T19:12:19Z

packages/rest/test/unit/rest-application/rest-application.test.ts

@@ -37,30 +37,13 @@ describe('RestApplication', () => {
      );
    });

-    it('when attempting to bind servers via configuration', () => {


I think we should re-write this test in another way or remove the code that deals with it if it no longer applicable. I'm thinking we create an application instance and call app.server after registering the rest component with app.component to get the no multiple servers allowed error.

I think your proposed test case is covered by the one below if you're concerned about making sure it errors out when RestComponent is registered with RestApplication. If you're concerned about throwing ERR_NO_MULTI_SERVER when another server is registered, that case is covered by the test above. Or I could be misunderstanding what test you want to replace this one with.

Yep the coverage I want is already there :-).

b-admike

LGTM. IMO the UX for our users would be easier with these changes.

virkt25

Great job but in terms of UX I had a comment earlier in boot PR from @bajtos IIRC (I'm unable to find a reference -- it might've been on the first PR I closed) ... but in essence we shouldn't be introducing methods where the distinguisher is an s at the end of a similar function name.

It is possible for us to overload methods which keeping type-safety. You can find an example here: https://github.com/strongloop/loopback-next/blob/e6490389decf21e40558ffb131cc5c3de5b1622a/packages/core/src/application.ts#L70-L97

I think we should make the APIs (methods) consistent ... whether that's me adding a booters or these functions dropping the s.

Thoughts @strongloop/sq-lb-apex @bajtos @raymondfeng ?

shimks · 2018-02-07T21:50:44Z

Either way is fine with me. Originally, I planned on overloading the methods, but I discovered that there were already servers function on top of server so I thought I'd stick to the format.

I'm for the overloading if I was forced to choose.

bajtos

+1 on @virkt25's comment about avoiding method names differing by the last s only (controller vs controllers).

I don't have a strong opinion whether it's better to have an overloaded controller method accepting both a single controller and an array of controllers, or to rename controllers to something more distinctive from singular controller() method.

bajtos · 2018-02-08T07:04:06Z

packages/core/test/unit/application.test.ts

@@ -147,3 +137,5 @@ class FakeServer extends Context implements Server {
    this.running = false;
  }
 }
+
+class FakerServer extends FakeServer {}


FakeServer and FakerServer are difficult to distinguish. Can you rename this class to AnotherServer please?

bajtos · 2018-02-08T07:08:24Z

@shimks the rest of the proposed changes look great! 👍

raymondfeng · 2018-02-08T18:47:41Z

My ideal signature will be something like:

// Public signatures
controller(ctor: ControllerClass, name?: string): Binding;
controller(...ctors: ControllerClass[]): Bindings[];

// Implementation
controller(
  ctor: ControllerClass, // First ctor
  nameOrCtor?: string | ControllerClass, // Name or 2nd ctor
  ...moreCtors: ControllerClass[] // Other ctors
) {
  const ctors: ControllerClass[] = [];
  let name: string | undefined = undefined;
  if (ctor) ctors.push(ctor);
  if (nameOrCtor && typeof nameOrCtor !== 'string') {
    ctors.push(nameOrCtor, ...moreCtors);
  } else if (typeof nameOrCtor === 'string') {
    name = nameOrCtor;
  }
  for (const c of ctors) {
    // ...
  }

This way, we can call:

app.controller(MyController, 'my-controller');
app.controller(MyController);
app.controller(MyController, AnotherController);

I admit the implementation is a bit tricky. Just an idea for consideration.

shimks · 2018-02-08T18:52:18Z

I'd be ok with that kind of change as long as everybody else is. @strongloop/sq-lb-apex What do you think?

virkt25 · 2018-02-08T20:44:52Z

Hmm ... so a few suggestions. Trying to think of this from a UX perspective ...

As a user if I wanted to pass in multiple Controllers, passing it into a controller method seems weird. But I think it's slightly better to pass in one Controller into a controllers method.

@raymondfeng I like your proposal to just pass in a list of Constructors instead of an Array since it's a much better UX, much cleaner than an array! +1 for that. I think to make it a little more cleaner, would it be better for us to completely get rid of the optional name string that we use for binding. I'm not sure of the use case of providing our own name for binding.

@loopback/boot will default to Constructor name, so I think we should stick to that and only that.

raymondfeng · 2018-02-08T20:49:47Z

The name is useful to bind the same class (especially RestServer) to multiple keys with different options.

BTW, we get rid of it if #929 is landed. #929 will allow app to accept a prebuilt-binding, which can have key and other attributes.

b-admike · 2018-02-08T21:05:00Z

I'm okay with either approaches. I see that as a user, if I try to register a bunch of controllers, using app.controllers([...]) is intuitive. I also understand that having controller vs controllers might be confusing, but I like the intuitive approach and separation. @bajtos to me controllers is a good description of the method. What names did you have in mind?

raymondfeng · 2018-02-08T21:12:31Z

I vote for the singular form app.controller() as long as it accepts a single controller.

virkt25 · 2018-02-08T21:34:40Z

I think I'm ok with having both app.controller() and app.controllers() ... with controllers taking an array or @raymondfeng 's design of just classes.

bajtos · 2018-02-09T08:49:18Z

to me controllers is a good description of the method. What names did you have in mind?

controllers would be a perfect description if there wasn't another method called controller.

Usually, method names start with a verb (e.g. registerController), in which case I would suggest names like registerController vs. registerManyControllers.

In this particular case, I am running out of ideas to be honest. app.manyControllers looks weird to me. Maybe it's a sign that we should start naming methods with verbs again? In which case I am proposing app.controller (for simplicity and backwards compatibility) and app.registerManyControllers.

I like the idea of using variable number of arguments instead of passing in an array. With ES2015+, one can use spread operator to easily convert between these two forms:

const controllersList = [ProductController, CategoryController];
app.registerManyControllers(...controllersList);
// the line above is equivalent to the line below
app.registerManyControllers(ProductController, CategoryController);

bajtos · 2018-02-09T12:12:37Z

I vote for the singular form app.controller() as long as it accepts a single controller.

I am voting for this option too.

// must have
app.controller(ProductController);
app.controller(MyProductController, 'ProductController');

// should have (?)
app.controller(ProductController, CategoryController);

On the second thought, I am not convinced that it's necessary to a single method to register multiple controllers. Does anybody remember why this requirement was created? @raymondfeng @virkt25 @kjdelisle

I am personally fine with a fluent API instead:

  app
    .controller(ProductController)
    .controller(CategoryController);

If we want to allow the user to customize the binding instance created for the controller, then the following form is ok with me too:

app.controller(ProductController);
app.controller(CategoryController);

Most applications will be using convention-based @loopback/boot, I am expecting app.controller to become a kind of an internal API for the majority of our users.

It seems to me that this PR could benefit from splitting into two parts:

Decouple application config from features. AFAICT, this change is non-controversial and can be landed quickly. It's also a breaking change that can create merge conflicts in other pull requests (e.g. my own refactor: simplify app setup via RestApplication (part II) #968), therefore we would all benefit from landing it ASAP.
Add array-based APIs for registering artifacts like controllers. This part requires discussion about API design and user experience, therefore we should take a slower approach.

shimks · 2018-02-09T15:40:39Z

I've pulled backed the last two commits so that the decoupling of app config portion of the PR can be safely landed. The commits still include accepting arrays through components (plural), but I may create another commit to reverse this.

bajtos · 2018-02-09T17:52:20Z

The commits still include accepting arrays through components (plural), but I may create another commit to reverse this.

Yes please, I would prefer it this way. But if everybody else thinks plural methods are ok, then I suppose I can live with them temporarily, until a better solution is found.

I am afraid I was faster to land #968 than you finishing this pull request, please rebase your PR on top of the latest master and fix merge conflicts. I hope they won't be too difficult to resolve.

bajtos

I would be much much happier if we could leave app.controllers and app.components out of this pull request.

virkt25 · 2018-02-09T19:53:50Z

Does anybody remember why this requirement was created?

This requirement came about from something I might've said because when the issue was originally created, we hadn't figured out boot and I wasn't happy about removing a singular location to pass in an array of classes for binding since calling app.controller() multiple times would've been frustrating and a bad UX.

Now that said, since boot is almost done and mostly eliminates the need for anyone using sugar methods for binding artifacts directly, I think it's probably a good idea to drop this requirement and just use what we had (app.controller(ctrl, name?)) ... especially if boot is a default component when creating a new project using CLI.

shimks · 2018-02-12T16:46:48Z

@slnode test please

bajtos

This is awesome now ✌️

I especially like the diff stats: 106 new lines, 247 lines removed.

If we wish to count lines of code, we should not regard them as "lines produced" but as "lines spent".
-- E. W. Dijkstra

bajtos · 2018-02-13T09:58:54Z

packages/example-log-extension/README.md

@@ -35,10 +35,10 @@ import {
 class LogApp extends LogLevelMixin(RestApplication) {
  constructor() {
    super({
-      components: [LogComponent],
      logLevel: LOG_LEVEL.ERROR,


This logLevel configuration may need update as part of #952 or after it's landed. I am not sure, I'd just like to bring this to our attention - I guess mostly yours, @raymondfeng .

shimks requested review from bajtos, kjdelisle, raymondfeng and virkt25 as code owners February 7, 2018 18:14

shimks added the needs-priority label Feb 7, 2018

shimks changed the title ~~Decouple config~~ Decouple application config from features (e.g. components) Feb 7, 2018

kjdelisle approved these changes Feb 7, 2018

View reviewed changes

b-admike reviewed Feb 7, 2018

View reviewed changes

b-admike approved these changes Feb 7, 2018

View reviewed changes

shimks mentioned this pull request Feb 7, 2018

feat(lb4): decouple app config from features loopbackio/loopback.io#613

Merged

virkt25 suggested changes Feb 7, 2018

View reviewed changes

bajtos suggested changes Feb 8, 2018

View reviewed changes

shimks force-pushed the decouple-config branch 3 times, most recently from 2884dda to 4dca2d1 Compare February 8, 2018 17:58

shimks mentioned this pull request Feb 8, 2018

fix(lb4): use overloaded functions for bindings loopbackio/loopback.io#615

Merged

shimks force-pushed the decouple-config branch from 4dca2d1 to 0ab3d26 Compare February 9, 2018 15:29

bajtos approved these changes Feb 9, 2018

View reviewed changes

shimks force-pushed the decouple-config branch from 0ab3d26 to cecf9ff Compare February 9, 2018 19:07

shimks force-pushed the decouple-config branch from ef31f64 to cecf9ff Compare February 12, 2018 16:48

virkt25 approved these changes Feb 12, 2018

View reviewed changes

shimks force-pushed the decouple-config branch from 8ed694a to cecf9ff Compare February 13, 2018 00:16

bajtos mentioned this pull request Feb 13, 2018

feat: @loopback/boot #858

Merged

6 tasks

bajtos approved these changes Feb 13, 2018

View reviewed changes

shimks force-pushed the decouple-config branch from cecf9ff to e22a717 Compare February 13, 2018 15:34

shimks added 5 commits February 13, 2018 10:36

fix(core): decouple app config from assembly

05af2cf

fix(core): update the readmes

d6e51e4

fix(core): add comments

3918447

fix(rest): rebase against RestApplication PR

0101887

fix(core): get rid of array registration

8d33bec

shimks force-pushed the decouple-config branch from e22a717 to 8d33bec Compare February 13, 2018 15:36

shimks merged commit 707a401 into master Feb 13, 2018

shimks deleted the decouple-config branch February 13, 2018 16:10

shimks removed the needs-priority label Feb 13, 2018

This was referenced Feb 13, 2018

fix: clean up example-log-extension #952

Merged

Should we introduce sugar for bulk app bindings? #990

Closed

Decouple wiring of implementation bits from application configuration #742

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple application config from features (e.g. components) #975

Decouple application config from features (e.g. components) #975

shimks commented Feb 7, 2018 •

edited

Loading

kjdelisle left a comment

kjdelisle Feb 7, 2018

kjdelisle Feb 7, 2018

shimks Feb 7, 2018 •

edited

Loading

b-admike Feb 7, 2018

shimks Feb 7, 2018 •

edited

Loading

b-admike Feb 7, 2018

shimks Feb 7, 2018

b-admike left a comment

virkt25 left a comment

shimks commented Feb 7, 2018

bajtos left a comment

bajtos Feb 8, 2018

bajtos commented Feb 8, 2018

raymondfeng commented Feb 8, 2018

shimks commented Feb 8, 2018

virkt25 commented Feb 8, 2018

raymondfeng commented Feb 8, 2018 •

edited

Loading

b-admike commented Feb 8, 2018

raymondfeng commented Feb 8, 2018

virkt25 commented Feb 8, 2018

bajtos commented Feb 9, 2018

bajtos commented Feb 9, 2018

shimks commented Feb 9, 2018 •

edited

Loading

bajtos commented Feb 9, 2018

bajtos left a comment

virkt25 commented Feb 9, 2018

shimks commented Feb 12, 2018

bajtos left a comment •

edited

Loading

bajtos Feb 13, 2018

Decouple application config from features (e.g. components) #975

Decouple application config from features (e.g. components) #975

Conversation

shimks commented Feb 7, 2018 • edited Loading

Checklist

kjdelisle left a comment

Choose a reason for hiding this comment

kjdelisle Feb 7, 2018

Choose a reason for hiding this comment

kjdelisle Feb 7, 2018

Choose a reason for hiding this comment

shimks Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

b-admike Feb 7, 2018

Choose a reason for hiding this comment

shimks Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

b-admike Feb 7, 2018

Choose a reason for hiding this comment

shimks Feb 7, 2018

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

virkt25 left a comment

Choose a reason for hiding this comment

shimks commented Feb 7, 2018

bajtos left a comment

Choose a reason for hiding this comment

bajtos Feb 8, 2018

Choose a reason for hiding this comment

bajtos commented Feb 8, 2018

raymondfeng commented Feb 8, 2018

shimks commented Feb 8, 2018

virkt25 commented Feb 8, 2018

raymondfeng commented Feb 8, 2018 • edited Loading

b-admike commented Feb 8, 2018

raymondfeng commented Feb 8, 2018

virkt25 commented Feb 8, 2018

bajtos commented Feb 9, 2018

bajtos commented Feb 9, 2018

shimks commented Feb 9, 2018 • edited Loading

bajtos commented Feb 9, 2018

bajtos left a comment

Choose a reason for hiding this comment

virkt25 commented Feb 9, 2018

shimks commented Feb 12, 2018

bajtos left a comment • edited Loading

Choose a reason for hiding this comment

bajtos Feb 13, 2018

Choose a reason for hiding this comment

shimks commented Feb 7, 2018 •

edited

Loading

shimks Feb 7, 2018 •

edited

Loading

shimks Feb 7, 2018 •

edited

Loading

raymondfeng commented Feb 8, 2018 •

edited

Loading

shimks commented Feb 9, 2018 •

edited

Loading

bajtos left a comment •

edited

Loading