Skip to content
This repository has been archived by the owner on Feb 2, 2018. It is now read-only.

Development #1

Closed
wants to merge 4 commits into from
Closed

Development #1

wants to merge 4 commits into from

Conversation

virkt25
Copy link
Collaborator

@virkt25 virkt25 commented Sep 22, 2017

Initial PR adding templates for Mixins, Controllers, Repositories.

Need to do:

  • Still need to add Providers & Components templates
  • I know the original scope of this task called for meaningful tests but I'm not sure how we can write any tests for what is essentially a template to make getting started easier.
  • Move documentation from code files to README
  • Controller with basePath config done using Dependency Injection (Do we need a template for this ... ?)

Questions

  • Not sure about the repository template since it more or less looks like the interface for CRUD for them to get started.
  • Is there an approach with controllers we want to recommend / promote
  • Thoughts on splitting each template into a different folder? -- And we can add the appropriate build config for each template there ... so they copy the desired folder instead of most likely cloning this repo and deleting the unwanted templates. This will lead to duplicated config code.

@bajtos Am I on the right track? I've been trying to create something abstract since it's meant to be a template to start from. Thoughts / comments / changes appreciated :)

connect to loopbackio/loopback-next#525

@virkt25 virkt25 requested review from bajtos and kjdelisle September 22, 2017 05:08
@bajtos
Copy link
Contributor

bajtos commented Sep 22, 2017

Hey @virkt25, thank you for wrapping the work and sending this initial pull request.

First of all, I see the project infrastructure is not fleshed out yet, e.g. there is are no npm scripts for compiling and linting the code. I'll take care of that myself and open another pull request shortly.

I would like to make this repo public, so that I can configure Travis CI integration. Any objections?

I know the original scope of this task called for meaningful tests but I'm not sure how we can write any tests for what is essentially a template to make getting started easier.

As I am envisioning this template, it should provide a meaningful implementation/example of each entity. For example, for a custom sequence action, we can use the Common Log logger described in Thinking in LoopBack. I believe that a concrete implementation will naturally show us what tests need to be written.

Let me put this into a wider context. As framework developers, we have responsibility not only to show our users how to use framework features to write extensions, but also what's the right process for building applications and extensions. I hope the necessity to have a good automated test coverage is a given thing that everyone understand these days. To focus on this starter repo, how are we going to know that our templates work as expected, when there are no tests to verify that claim? When other people start contributing changes, or even when we contribute changes in several months, how are we going to tell whether a pull request is going to break something or not, if there are no automated tests?

I haven't thought about detailed testing strategy for extensions yet. At minimum, I think we should have two kinds of tests:

  • Acceptance/end-to-end tests exercising customizations provided by the extension in a setup that's close to real-world usage: an Application using the extension, a client making HTTP/gRPC/etc. calls. This is needed to verify that the extension is easy to setup, configure and use; and also that all moving parts fit and work together. (The example in Thinking in LoopBack shows this kind of a test.)

  • Unit or integration tests exercising the component in (partial) isolation, these tests should cover different aspects, execution paths and edge cases. (The example in Thinking in LoopBack asks the reader to write such test to verify that the logger handles rejected/failed requests too.)

Move documentation from code files to README

+1

I think some of this documentation should actually go to http://loopback.io/doc/en/lb4/Creating-components.html. IMO, READMEs and code comments should describe things specific to this starter repository only. Content relevant to all extension developers should go to our docs.

Controller with basePath config done using Dependency Injection (Do we need a template for this ... ?)

IIRC, basePath is actually not supported by our REST transport yet. However, if I understand your scenario correctly, it's a valid and important one, something we should make easy in our framework. I think it's out of scope of this work though, could you please open a follow-up issue and include the task of updating this starter repo in the acceptance criteria?

Not sure about the repository template since it more or less looks like the interface for CRUD for them to get starte

Hmm 🤔

Here is the use-case I have in mind: let's say we are using a microservice architecture and have multiple services accessing the same source of data - for example a weather service or perhaps an employee database. As a developer in such organization, I would like to have an extension that gives me models, repositories and datasource(s) that I can easily plug into my application to leverage this shared source of data in my microservice.

To make this self-contained and independent on external services, I am proposing to leverage memory connector, configure it to load/save data to a file (see file config options). The data can be something that's usually read-only, for example a list of country codes and names? To keep the example simple, let's expose a full CrudRepository allowing consumers to modify this data too. Can we come up with a better example? Something where it makes sense to have initial seed data shared by all dependants, but where it also makes sense for each dependant to edit its copy. Perhaps some sort of a pre-populated cache?

Is there an approach with controllers we want to recommend / promote

Can you elaborate more on this please?

In my mind, a controller shared via an extension would provide a common piece of REST API functionality. For example, an authentication component can provide a controller providing API for managing local users (create, edit profile, change password, etc.).

Thoughts on splitting each template into a different folder? -- And we can add the appropriate build config for each template there ... so they copy the desired folder instead of most likely cloning this repo and deleting the unwanted templates. This will lead to duplicated config code.

-1

I believe most components will contribute more than just one thing. For example, the authentication component is exporting few bindings (providers) and a sequence action (as a provider). I think eventually it may provide controller for local user management and more.

In that case, I think it will be difficult to build such a component if one has to copy and merge multiple templates together.

I am proposing to come up with a directory layout convention that works for both extensions and applications. I have already started on while working on Thinking in LoopBack - see here.

screen shot 2017-09-22 at 13 39 29

In my upcoming pull request adding project infrastructure, I'll setup these subdirectories for you.

Am I on the right track? I've been trying to create something abstract since it's meant to be a template to start from.

I'll admit I haven't reviewed details of your controller/mixin/repository templates yet, as I sort of got stuck with the overall project layout/setup and would like to get it right first.

I think creating an abstract template makes this work unnecessary difficult, as you have already discovered when you wanted to write a unit test.

IMO, we should also use this task and the starter repository as an opportunity to check the user experience of consuming different kinds of extensions (depending on what they are contributing) and make sure our design provides great UX for extension consumers (and extension authors too). I think this check is easier when we focus on specific use cases that naturally show us the limitations of our design, rather than try to address a vague/abstract set of all possible use cases.

This Mixin starter will show you how to extend loopback-next Application OR
any other class using the concept of a Mixin. A Mixin is a class which takes the
input of a base class and adds / modifies existing functions / static values and returns
a new class which can be instantiated. The type of returned class will still be the base class.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Let's keep this template focused and show how to write a component mixin for Application.
  2. This file should not be explaining the general concept of mixins, but point people to our documentation instead. Ideally, there should be a section in "Writing Components" explaining how to write a mixin extending application object, we should refer to that section from this file.

export interface Constructor<T> {
new (...args: any[]): T;
[property: string]: any;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this interface should be inherited from a shared loopback package or from @loopback/core.

@virkt25 virkt25 changed the title Devlopment Development Sep 22, 2017
@virkt25
Copy link
Collaborator Author

virkt25 commented Sep 25, 2017

Hi @bajtos, Thanks for all the insightful comments.

Since the starter template is going to be more along the lines of a completed example template, I'm thinking of doing 2 extensions. First being a @count decorator for controller operations which when applied will track the number of times a given endpoint is called. This will be stored in the memory repository and a /count or /stats endpoint will show the number of times each endpoint has been called ... and then the appropriate CRUD endpoints will be available for users to view count for individual endpoints, update count, delete / reset count, etc.

A second one being a logging component that'll track parameters given to a controller operation function, and the result it produces. Or something along the lines of what is described in Thinking in Loopback ... but with the difference of everything being stored in the DB and CRUD endpoints made available for logs. -- This can be an extension along the lines of @log or a sequence action.

Thoughts on this?


I'm running into some issues with creating a DefaultCrudRepository as specified here: https://github.com/strongloop/loopback-next/tree/master/packages/repository#define-a-repository
I'm not sure what's missing from my implementation seen here: https://github.com/strongloop/loopback-next-extension-starter/blob/0db67f9ba1061fadf39e5d41caea54bd5a8c3bb8/src/repositories/index.ts#L11-L27

I get the following error:

src/repositories/index.ts(25,11): error TS2345: Argument of type 'typeof PersistedModel' is not assignable to parameter of type 'typeof Entity & { prototype: Entity; }'.
  Type 'typeof PersistedModel' is not assignable to type 'typeof Entity'.
    Property 'buildWhereForId' is missing in type 'typeof PersistedModel'.

Any idea as to what I might be doing wrong? There is commented out code similar to loopback-next-example for a repository that works but it's different than the README.

@bajtos
Copy link
Contributor

bajtos commented Sep 25, 2017

Merge branch 'master' into devlopment

Please use git rebase master && git push -f in the future. We are keeping a linear history and when you merge master into devlopment, the conflicts resolved as part of that merge may need another resolution later, when you are cleaning up the git history.

file: './data.json',
});

const Count = ds.createModel<typeof juggler.PersistedModel>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See http://loopback.io/doc/en/lb4/Thinking-in-LoopBack.html#define-product-model-repository-and-data-source.

The model is defined as a regular class extending from Entity, no datasource is involved. The DefaultCrudRepository will create datasource-specific model under the hood.

@model()
export class Count extends Entity {
  @property({type: 'string', id: true, required: true}
  id: string;

  // ...
}

@@ -3,6 +3,7 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import 'mocha';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? This should not be needed.

@bajtos
Copy link
Contributor

bajtos commented Sep 25, 2017

Since the starter template is going to be more along the lines of a completed example template, I'm thinking of doing 2 extensions. First being a @count decorator for controller operations which when applied will track the number of times a given endpoint is called. This will be stored in the memory repository and a /count or /stats endpoint will show the number of times each endpoint has been called ... and then the appropriate CRUD endpoints will be available for users to view count for individual endpoints, update count, delete / reset count, etc.

A second one being a logging component that'll track parameters given to a controller operation function, and the result it produces. Or something along the lines of what is described in Thinking in Loopback ... but with the difference of everything being stored in the DB and CRUD endpoints made available for logs. -- This can be an extension along the lines of @log or a sequence action.

I am a bit concerned that your proposed examples are complex and require many dependencies, which may make it difficult for people using these examples as a starter template to figure out what parts are important (required for a decorator/sequence action/whatever) and what is implementation detail of our particular example.

Can we come up with something simpler and more lightweight please?

The starter repo should explain/show the following items:

  • File layout convention - where to put which file
  • Design - how to write a decorator/binding/sequence action (use a class? inherit from something?)
  • Integration/exports - how to expose decorators/bindings/sequence actions to make them easy to consume from applications
  • Testing - how to write a good test suite, what conventions and best practices to follow

The rest is an implementation details that we should de-emphasize, because these details will be different in each extension.


As I am thinking about decorators, they are tricky. The way how they are typically used, a decorator only stores some metadata that's later used by a different piece of code. Maybe we should have two example decorators - a simple one providing a wrapper for an existing decorator to show how to expose decorators from components and test them, and a more complex one focusing more on how to implement both a decorator and the code consuming metadata from this decorator.

A proposal for the simple decorator:

/**
 * A parameter containing the current transaction-id provided
 * via HTTP request header "X-Transaction-Id".
 */
export function transactionIdHeader() {
  return param.header.string('X-Transaction-Id');
}

The complex decorator can build on top of your proposal, but let's simplify it: return the number of calls in the response header (instead of exposing a new HTTP endpoint, which would require a controller), and store the data using plain javascript object/Map (instead of bringing in heavy-weight juggler). I think you will need a custom sequence action in order to implement this feature too.

As for the logger - the idea is to show how an extension can provide a sequence action. Again, let's keep this simple and focused on the mechanics of sequence actions. IMO, printing the logs to console is good enough.

@raymondfeng
Copy link
Contributor

@bajtos I agree with your comments. The purpose of loopback-next-hello-extension is to give extension developers a very simple example to help them understand the project layout and how extensions such as decorators, binding providers, and actions can be contributed.

Let's leave the implementation as trivial as possible. For example, it's perfectly fine to use memory to keep stats.

Copy link
Collaborator Author

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attempted to add a @transactionIdHeader decorator but was unsure how to get access to request in the decorator without having it inject from Context ... which also didn't work.

Any hints / ideas / suggestions? Why can't I access context / How would I access context in a decorator?

Any ideas? In the meantime I started a new PR from scratch that implements a @log decorator. See that for more details.

console.log('descriptor =>', descriptor);
return {
value: function(...args:any[]) {
const req: any = Reflector.getMetadata('http.request', )
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried writing a simple transactionIdHeader but ran into the issue of not being able to access the Context in a decorator to get the value for the header from http.request. Any ideas how this can be achieved? I'm guessing the right way for request associated transactions would be to do this through sequence?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the decorator implementation I had in mind:

export function transactionIdHeader() {
  return param.header.string('transactionId');
}

It delegates the implementation complexity to the built-in @param decorator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To answer your original question: the decorators cannot access any context, they are invoked at the time when the source code files are required (loaded by Node.js runtime), there is no context instance created at that time yet.

You can look at @param() implementation to understand how the param decorator stores metadata, and parseParams() sequence actions to understand how this metadata is used.


class MyController {
@get('/')
@transactionIdHeader()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also tried injecting http.request into transactionIdHeader() ... but that didn't work for obvious reasons.

@bajtos
Copy link
Contributor

bajtos commented Oct 12, 2017

This is no longer relevant.

@bajtos bajtos closed this Oct 12, 2017
@virkt25 virkt25 deleted the devlopment branch October 16, 2017 14:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants