Skip to content

feat(cli): Cleanup REST application tooling#774

Merged
shimks merged 7 commits intomasterfrom
cli-rest-project
Dec 8, 2017
Merged

feat(cli): Cleanup REST application tooling#774
shimks merged 7 commits intomasterfrom
cli-rest-project

Conversation

@shimks
Copy link
Copy Markdown
Contributor

@shimks shimks commented Dec 5, 2017

@shimks
Copy link
Copy Markdown
Contributor Author

shimks commented Dec 5, 2017

@slnode test please

"build:watch": "lb-tsc --watch",
"clean": "lb-clean",
<% if (project.prettier && project.tslint) { -%>
<% if (project.prettier && project.tslint) { -%>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need to change the formatting for this file. Please revert.

"build:watch": "tsc --outDir dist --target es2017 --watch",
"clean": "rm -rf dist",
<% if (project.prettier && project.tslint) { -%>
<% if (project.prettier && project.tslint) { -%>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need to change the formatting for this file. Please revert.

If you have a build script to do auto-formatting, please exclude template files.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@raymondfeng With the previous formatting, the indenting on the generated package.json was inconsistent. The indenting is consistent with the new format.

{
"$schema": "http://json.schemastore.org/tsconfig",
<% if (project.loopbackBuild) { -%>
<% if (project.loopbackBuild) { -%>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need to change the formatting for this file. Please revert.

{
"$schema": "http://json.schemastore.org/tslint",
<% if (project.loopbackBuild) { -%>
<% if (project.loopbackBuild) { -%>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need to change the formatting for this file. Please revert.

// Note: arguments and options should be defined in the constructor.
constructor(args, opts) {
super(args, opts);
this._setupGenerator();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe it's better to remove redundant call of this._setupGenerator() in subclasses instead.

@@ -0,0 +1,3 @@
# Datasources

This directory contains code for datasources provided by this app.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest rephrasing to contains config for datasources used by this app.

Copy link
Copy Markdown
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

:shipit:

} from '@loopback/rest';
import {ServerResponse} from 'http';

export class MySequence extends DefaultSequence {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can't extend a Sequence because of DI ... we can't inject new sequence actions if we do this. Below is a sample implementation from log-extension I would suggest using (removing the logging stuff).

    class LogSequence implements SequenceHandler {
      constructor(
        @inject(RestBindings.Http.CONTEXT) public ctx: Context,
        @inject(SequenceActions.FIND_ROUTE) protected findRoute: FindRoute,
        @inject(SequenceActions.PARSE_PARAMS)
        protected parseParams: ParseParams,
        @inject(SequenceActions.INVOKE_METHOD) protected invoke: InvokeMethod,
        @inject(SequenceActions.SEND) protected send: Send,
        @inject(SequenceActions.REJECT) protected reject: Reject,
        @inject(EXAMPLE_LOG_BINDINGS.LOG_ACTION) protected logger: LogFn,
      ) { }

      async handle(req: ParsedRequest, res: ServerResponse) {
        // tslint:disable-next-line:no-any
        let args: any = [];
        // tslint:disable-next-line:no-any
        let result: any;

        try {
          const route = this.findRoute(req);
          args = await this.parseParams(req, route);
          result = await this.invoke(route, args);
          this.send(res, result);
        } catch (err) {
          this.reject(res, req, err);
          result = err;
        }

        await this.logger(req, args, result);
      }
    }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@virkt25 Are you saying @inject doesn't allow inheritance from base classes?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes. I get the following error message

Uncaught Error: Cannot resolve injected arguments for function MySequence: The arguments[0] is not decorated for dependency injection, but a value is not supplied

When I try using the following MySequence (in log-extension acceptance test) ... when I'm trying to inject a custom sequence action.

class MySequence extends DefaultSequence {
      constructor(
        public ctx: Context,
        protected findRoute: FindRoute,
        protected parseParams: ParseParams,
        protected invoke: InvokeMethod,
        public send: Send,
        public reject: Reject,
        @inject(EXAMPLE_LOG_BINDINGS.LOG_ACTION) protected logger: LogFn,
      ) {
        super(ctx, findRoute, parseParams, invoke, send, reject);
      }

      // Handle your request routing here:
      async handle(req: ParsedRequest, res: ServerResponse) {
        let args: any = [];
        let result: any;

        try {
          const route = this.findRoute(req);
          args = await this.parseParams(req, route);
          result = await this.invoke(route, args);
          await this.send(res, result);
        } catch (err) {
          this.reject(res, req, err);
          result = err;
        }

        await this.logger(req, args, result);
      }
    }

    server.sequence(MySequence);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm adding support for that in #775.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@raymondfeng I've made a commit with @virkt25 's suggestion in mind, but should I revert it if you're adding in support for inheritance in @inject?

@shimks
Copy link
Copy Markdown
Contributor Author

shimks commented Dec 6, 2017

@slnode test please

@shimks shimks merged commit dc50ed8 into master Dec 8, 2017
@shimks shimks deleted the cli-rest-project branch December 8, 2017 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants