Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spike: Create Route in JSLB4 #2474

Closed
5 tasks
hacksparrow opened this issue Feb 26, 2019 · 5 comments
Closed
5 tasks

Spike: Create Route in JSLB4 #2474

hacksparrow opened this issue Feb 26, 2019 · 5 comments

Comments

@hacksparrow
Copy link
Contributor

hacksparrow commented Feb 26, 2019

Follow up task of #1978.

Acceptance criteria

  • Should be created as a class

  • Should be useable with JSLB4 Application class

  • Should have access to:

    • The LB4 request object and metadata contributed by LB4 components eg: @loopback/authentication.
    • A Model Repository
    • The LB4 response object

    UPDATE 2019-03-12 @raymondfeng @hacksparrow and @bajtos discussed this requirement and agreed it's good enough to pass requestContext as the first argument to the handler function. We need to preserve backwards compatibility for handlers that are not expecting the context in their parameters.

@bajtos
Copy link
Member

bajtos commented Feb 26, 2019

(1)

Should be created as a class

This part does not make sense to me. Routes are implemented as functions, see e.g. here:

https://github.com/strongloop/loopback4-example-javascript/blob/c9c654934644da705b81eb3450f6450ee37df5b7/server/application.js#L15

    this.route('get', '/hi', {responses: {}}, () => 'Hi!');

(2)
In addition to the list of objects the route should have access to, the route must be also able to receive parameters parsed by LB4 from the request. The parameters are described in the provided OpenAPI spec (this.route argument number 3), either via parameters or requestBody.

(3)
Having access to a Model is not very useful, it needs to access a Repository instance that can be used query model's data and make any changes.

(4)
To describe route's inputs and outputs, we need a helper function to build JSON schema from LB4 model class. I believe that in TypeScript, this conversion is done automatically by @param and @response decorators. In JavaScript, we need to be more explicit.

// a mock-up I am typing from my head, may be wrong in spec details
this.route('post', '/products', {
  requestBody: {
    content: {
      'application/json': {
        schema: buildOpenApiSchemaForModel(Product)
      },
    },
  },
  responses: {
    '200': {
      description: 'Product created',
      content: 
        'application/json': {
          schema: buildOpenApiSchemaForModel(Product)
        },
    },
  },
}, (data) => productRepository.create(data));

I think we will need to address this part for controllers too (#2478) depending on which approach we choose.

(5)
As you can see, building the OperationObject requires a lot of boilerplate code. It would be great to create a follow-up spike to look for ways how to simplify this part. For example:

// a mock-up I am typing from my head, may be wrong in spec details
this.route('post', '/products', {
  requestBody: buildRequestBodySpecForModel(Product),
  responses: {
    '200': buildResponseSpecForModel(Product, 'ProductyCreated')
  },
}, (data) => productRepository.create(data));

@bajtos
Copy link
Member

bajtos commented Feb 26, 2019

See also #1978 (comment) for few ideas on how to make routes easy to implement in JavaScript.

@emonddr
Copy link
Contributor

emonddr commented Apr 4, 2019

@hacksparrow , we would like you to update the Acceptance Criteria based on the thread discussion. Thank you.

@stale
Copy link

stale bot commented Mar 29, 2020

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Mar 29, 2020
@stale
Copy link

stale bot commented Apr 29, 2020

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants