Sugar APIs for creating controllers, models, and repositories #3

hacksparrow · 2019-01-30T03:54:37Z

Sugar APIs for creating controllers, models, and repositories.

hacksparrow · 2019-01-31T17:56:02Z

@bajtos @raymondfeng here is the new LB4 sugar API for JS. Good thing, it preserves the underlying metadata info without exposing them to the user.

The following files are created by the user:

What makes them possible: https://github.com/strongloop/loopback4-example-javascript/tree/hack/lib

The codebase can be optimized some more. Before that, I want your feedback on whether this is the right direction or not. Do you see any potential issues, how does it look?

jannyHou

@hacksparrow Great effort of creating the suger generator functions!
I cloned your branch and run on local, verified that the application starts perfectly and shows the explorer 👍

I also tried some endpoints, like GET /color/count, which works fine, while got 500 internal server error when try to create a color instance by POST /color with payload

{
  "id": 200,
  "value": "string"
}

This is not a complain of some endpoints not working, I totally understand the effort needed to generate a perfect working app :)

My point is, based my trial, the next thing I would like to do is taking a look of the generated controller file like the typescript version TodoController.ts, the generator approach is more like LB3 style that we hide the implementation point for users. I am wondering are we going to create CLIs to generate the js files of artifacts in the future so that users can see and modify those files directly?

Other than ^ I like this Js example, the file structures are pretty straightforward and well organized.

jannyHou · 2019-01-31T18:14:29Z

server/controllers/ping.controller.js

+      'x-operation-name': 'pang',
+      'responses': {
+        '200': {
+          'description': 'POST Ping Response',


nitpick: POST Pong Response?

raymondfeng · 2019-01-31T22:50:24Z

@hacksparrow I like the idea of generating LB4 classes from plain JS. What about name these generators as *Factory?

hacksparrow · 2019-02-01T03:36:19Z

Thanks for the review, @jannyHou. It's working on my machine. Let me add some acceptance tests for everyone else to confirm.

hacksparrow · 2019-02-01T03:36:42Z

@raymondfeng good suggestion.

hacksparrow · 2019-02-01T06:14:49Z

The files under the lib dir should be published as an npm module, it will make for a much cleaner experience for the developers.

bajtos

My first reaction to the latest proposal: 😱

Besides the comments below, I am concerned that this proposal is focused on supporting very narrow subset of possible usage of LB4 APIs that will not get us very far. For example, how can users configure additional model and repository methods or configure their controllers and repositories to receive additional dependencies via DI?

Let me cross-post your loopbackio/loopback-next#1978 (comment):

Technical implementations to figure out:

How to define models, additional metadata for model properties?

How to define controllers, how to provide OpenAPI spec for controller methods?

How to do dependency injection, and how to provide arguments?

As I see it, the proposal provides:

A solution for 1 with possible limitations - see my comments below.
A partial solution for 2 that comes with UX issues (see my comment below) and as a result you are proposing a CRUD Controller base class instead of scaffolding the CRUD controller in target project as it the case in TypeScript.
No easy-to-use solution for 3, at least AFAICT.

I believe that if we can find a decent solution for 2 and especially for 3, then no class factories will be needed.

Let's do few more iterations to find a better solution.

bajtos · 2019-02-01T13:31:54Z

lib/crud-controller-factory.js

+      this.controllerRepository = controllerRepository;
+    }
+    async create(entity) {
+      return await this.controllerRepository.create(entity);


We have intentionally designed LB4 in a way that gives full control over the REST API shape to app developers, our lb4 controller command is intentionally scaffolding entire CRUD API implementation.

Your proposal here is similar to what Raymond proposed in loopbackio/loopback-next#740. It's a valid feature, but not what we are looking for in this spike.

As I see it, the spike should show how JavaScript developers can implement CRUD controllers while preserving full control over the shape of the REST API and the underlying implementation. Most importantly, the following task must be easy to achieve:

add or remove request parameters

modify the type (and OpenAPI spec) of a request parameter

implement a custom pre-processing step executed before the repository method is invoked

implement a custom post-processing step executed after the repository method returns

bajtos · 2019-02-01T13:32:33Z

lib/crud-controller-factory.js

+      },
+    }),
+    param(0, rest.param.path.number('id')),
+    param(1, rest.requestBody()),


Is it possible to implement this built-in controller class via the custom-controller-factory?

bajtos · 2019-02-01T13:34:22Z

server/controllers/color.controller.js

-], ColorController);
-
-exports.ColorController = ColorController;
+exports.ColorController = crudControllerFactory('color');


As commented above, we are looking for a nice way how to keep the actual actual controller implementation & API spec inside this file.

bajtos · 2019-02-01T13:38:04Z

server/controllers/ping.controller.js

-  rest.get('/ping', {
-    responses: {
-      '200': PING_RESPONSE,
+const specifications = {


I have to problems with this approach:

We don't have a controller class, therefore it's difficult to share state between method calls, e.g. the public API method is calling out to a private helper shared with other API methods and wants to keep the state in a member property of the controller instance.

How do you envision injecting dependencies? Let's say that ping wants to receive RestBindings.URL and send it back in the response. Or perhaps AuthenticationBindings.CURRENT_USER to make this more realistic

You have not addressed my earlier comment JavaScript abstraction layer for LB4 #1 (comment)

The method implementation is far away from its metadata. In my experience, it's an anti-pattern that makes the code difficult to read and maintain. We should look for ways how to allow developers to keep both the implementation and the metadata together in one place.

bajtos · 2019-02-01T13:48:36Z

server/datasources/memory.datasource.js

-
-exports.MemoryDataSource = MemoryDataSource;
+const config = require('./memory.datasource.json');
+exports.MemoryDataSource = datasourceFactory('memory', config);


In TypeScript, we are introducing per-datasource classes for the following reasons:

To simplify bindings setup and conventional boot, which is relying on unique DataSource class names.

To allow datasources to receive their configuration via Dependency Injection.

To make it easier for developers to add additional custom APIs or modify behavior of existing DataSource methods.

IIUC your proposal, users cannot customize the class because it's created by the factory - it's taking the third item away. At which point I'd prefer to find a way how we can use base juggler.DataSource class and allow the javascript file to export such metadata that 1) booter creates the right datasource binding 2) the configuration is provided to the datasource constructor.

This is a problem that's not specific to JavaScript only, it's a legitimate requirement to bind multiple instances of the same class but with a different config. See e.g. loopbackio/loopback-next#2259 support binding config and @inject.config

I'd much rather find a clean solution that works both for TS and JS codebases, rather than trying to implement a datasource-specific hack like what is proposed here.

I'd much rather find a clean solution that works both for TS and JS codebases, rather than trying to implement a datasource-specific hack like what is proposed here.

Or even better, find out an easy-to-use solution for configuring Dependency Injection from JavaScript (because we need it anyway) and then we can keep the existing code structure.

bajtos · 2019-02-01T14:01:20Z

server/models/color.model.js

+    }
+  },
+  // Supporting these additional features would be a lot of additional work
+  // and some features are still not available in LB4, yet


Let's forget about LB3 model definition please. In LB4, we have slightly different interface - see ModelDefinitionSyntax.

People building models from JavaScript can (and IMO should) use ModelDefinition class.

As a general rule I'd like us to follow, our TS APIs should not rely on decorators only, they should always provide a reasonable non-decorator path too. (A side note: decorators are an experimental API, the standardization in TC39 was put on hold. If decorators ever make it into official JavaScript language, their shape may be very different from what we have now.)

LB4 already contains examples on how to create models without decorators, see https://github.com/strongloop/loopback-next/blob/91a37dca4214bb8d0d3cdfbbcf3b64fbd7e7cae2/packages/repository/test/unit/model/model.unit.ts#L41-L58

The example I pointed to can be easily converted to JavaScript as follows:

const addressDef = new ModelDefinition('Address'); addressDef .addProperty('street', 'string') .addProperty('city', 'string') .addProperty('state', String) .addProperty('zipCode', STRING); class Address extends Entity { static definition = addressDef; constructor(data?: Partial<Address>) { super(data); } }

Personally, I would use the following version:

class Address extends Entity { static definition = new ModelDefinition('Address') .addProperty('street', 'string') .addProperty('city', 'string') .addProperty('state', String) .addProperty('zipCode', STRING); constructor(data?: Partial<Address>) { super(data); } }

IMPORTANT

LB2/LB3 users were asking for the ability to define models as regular ES6 classes for a long time. The new LB4 design makes that possible in TypeScript, we should look for ways how to preserve that in JavaScript too.

A typical scenario to consider: how to add a new helper method to a model class.

class Address extends Entity { static definition = new ModelDefinition('Address') .addProperty('street', 'string') .addProperty('city', 'string') .addProperty('state', String) .addProperty('zipCode', STRING); constructor(data?: Partial<Address>) { super(data); } // Return the address as a multi-line string, e.g. // 1 New Orchard RdArmonk // NY 10504 getFormattedAddress() return [this.street, this.city, `${this.state} ${this.zipCode}`].join('\n'); } }

Uh oh, JavaScript does not support property initialization syntax I used above :(

Here are two option how to work around that.

Option 1

const addressDef = new ModelDefinition('Address'); // add properties, etc. class Address extends Entity { static get definition() { return addressDef; } }

Option 2

class Address extends Entity { } Address.definition = new ModelDefinition('Address') // add properties, etc.

FWIW, static property initializers are at Stage 3 in TC39, see https://github.com/tc39/proposal-static-class-features

Slightly related: https://github.com/tc39/proposal-class-fields

bajtos · 2019-02-01T14:14:01Z

lib/model-factory.js

+    }
+    decorate([
+      repository.property(repositoryProperties),
+      metadata('design:type', typeMap[property.type])


I would like LB4 to work without design:type metadata too. Where are we relying on design:type of model properties? I'd like us to rework those places to use the real model definition instead. IMO, the only place accessing design-type metadata of model properties is the @property and @model decorators that are building the actual model definition.

design:type is only used for inference/inspection of TS types and they can/should be always overridable.

bajtos · 2019-02-01T14:14:26Z

lib/model-factory.js

+
+  Model = decorate([
+    repository.model(),
+    metadata('design:paramtypes', [Object])


This design:paramtypes entry shouldn't be needed at all, right? Can we remove it please?

Not need in all the cases, but in some places it is a breaker not to have them. Eg: https://github.com/strongloop/loopback-next/blob/master/packages/openapi-v3/src/controller-spec.ts#L203.

This file is not supposed to be exposed to the users, having additional consistent code should not hurt, IMO.

bajtos · 2019-02-01T14:41:42Z

I am proposing the following next steps:

Find out how to allow JS users to define their models as shown in Sugar APIs for creating controllers, models, and repositories #3 (comment), i.e. fix all parts of LB4 framework to support models with no design:type metadata.
Look into @inject alternative for JavaScript. Ideally, we need to support the following three flavours: 1) constructor arguments 2) class properties 3) arguments of a class method. My opinion on the priorities: constructor arguments = MUST HAVE, class properties = SHOULD HAVE, method arguments = NICE TO HAVE.

For DI configuration, I'd like to consider approach based on what we have for model definition, where the additional metadata is stored in static property. For example:

class MyController {
  /** property dependencies **/
  get $propertyDependencies() {
    return {
      // inject dependencies of our parent classes
      ...this.propertyDependencies,
      // configure the property `myController.url`
      url: RestBindings.URL
    };
  }

  /** constructor dependencies **/

  constructor(currentUser) {
    this.currentUser = curentUser;
  }
  static get constructorDependencies() {
    return [AuthenticationBindings.CURRENT_USER];
  }

  /** method-level dependencies **/
  
  getProfile(currentUser) {
    return currentUser;
  }
  
  get getProfileDependencies() {
    return [AuthenticationBindings.CURRENT_USER];
  }

  /** see how it can be used for OpenAPI spec too! **/

  get getProfileSpec() {
    return { 
     responses: {
        200: {
          'application/json': {
            schema: getUserProfileSchema();
          }
        }
     }
  }

I expect my proposal to be controversial and I am happy to consider different approaches and options.

For example, we can use OpenAPI spec (OperationObject) to define method parameters to be injected. The caveat: these injected parameters must be removed from OpenAPI spec before it's returned by the app (e.g. at GET /openapi.json).

raymondfeng · 2019-02-01T18:17:27Z

Great discussion!

Here is how I see it:

LB4 introduced a set of constructs/artifacts, such as model, datasource, controller, and repository. Most of them are represented as classes with some metadata. They are then bound to the Context by boot, component, or explicit API calls. With dependency injection, these building blocks can be composed to create various user experiences such as exposing REST APIs or integrating with backend data and services.
We declare such constructs as TypeScript classes at the moment. Metadata are added by a few different ways:

Use decorators
Introspect TypeScript design time metadata (added by TSC)
Use class level properties

Most of TypeScript classes are generated/predefined. They are less magic/open than LB3 artifacts.

We need to define the canonical representation of such constructs (class + metadata), which become the unified input for LB4. At this point, they are transpiled JavaScript classes from TypeScript.
To entertain both Vanilla JS and TypeScript developers and minimize the gap, Sugar APIs for creating controllers, models, and repositories #3 is critical.
@hacksparrow's PR tries to add factories that can generate TypeScript-flavored canonical representation. If we think that's not the best middle ground, we need to come up an agnostic format for Sugar APIs for creating controllers, models, and repositories #3. Such format should be easy to produce by APIs, and sugar/convenient alternatives (such as decoration, JSON, metadata properties by convention).
We should also consider two flavors of constructs:

Strong-typed and fully declared classes
Weakly-typed and open classes (like LB3, it's also useful to generate from other forms of metadata, such as service-proxy specs - OAS, gRPC service spec, WSDL, GraphQL schema)

hacksparrow · 2019-02-04T12:10:08Z

@jannyHou, the app should work now, btw. It was caused by the hard-coded local path of the database file.

Hage Yaapa added 6 commits January 30, 2019 01:27

controller-generator

3814823

basic model

631349c

repository generator

2d8053c

datasource generator

b2a2532

sequence generator

7367492

crud controller

668928b

hacksparrow force-pushed the hack branch from 358f458 to c80d66b Compare January 31, 2019 18:07

non-crud controller generator

c033759

hacksparrow force-pushed the hack branch from c80d66b to c033759 Compare January 31, 2019 18:20

jannyHou reviewed Jan 31, 2019

View reviewed changes

Hage Yaapa added 2 commits February 1, 2019 11:06

*factory

74edf8b

better API

50aec28

Hage Yaapa added 2 commits February 1, 2019 12:00

limit color set

b22a376

init tests

2e1c4d3

bajtos suggested changes Feb 1, 2019

View reviewed changes

bajtos reviewed Feb 1, 2019

View reviewed changes

dhmlau closed this Sep 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sugar APIs for creating controllers, models, and repositories #3

Sugar APIs for creating controllers, models, and repositories #3

hacksparrow commented Jan 30, 2019

hacksparrow commented Jan 31, 2019 •

edited

Loading

jannyHou left a comment •

edited

Loading

jannyHou Jan 31, 2019

raymondfeng commented Jan 31, 2019

hacksparrow commented Feb 1, 2019

hacksparrow commented Feb 1, 2019

hacksparrow commented Feb 1, 2019

bajtos left a comment

bajtos Feb 1, 2019

bajtos Feb 1, 2019

bajtos Feb 1, 2019

bajtos Feb 1, 2019

bajtos Feb 1, 2019

bajtos Feb 1, 2019

bajtos Feb 1, 2019

bajtos Feb 1, 2019

bajtos Feb 1, 2019

bajtos Feb 1, 2019

raymondfeng Feb 1, 2019

bajtos Feb 1, 2019

hacksparrow Feb 4, 2019

bajtos commented Feb 1, 2019

raymondfeng commented Feb 1, 2019

hacksparrow commented Feb 4, 2019

Sugar APIs for creating controllers, models, and repositories #3

Sugar APIs for creating controllers, models, and repositories #3

Conversation

hacksparrow commented Jan 30, 2019

hacksparrow commented Jan 31, 2019 • edited Loading

jannyHou left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Jan 31, 2019

hacksparrow commented Feb 1, 2019

hacksparrow commented Feb 1, 2019

hacksparrow commented Feb 1, 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

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

bajtos commented Feb 1, 2019

raymondfeng commented Feb 1, 2019

hacksparrow commented Feb 4, 2019

hacksparrow commented Jan 31, 2019 •

edited

Loading

jannyHou left a comment •

edited

Loading