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: booter for creating REST APIs from model files #3617

Closed
wants to merge 13 commits into from

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Aug 29, 2019

Quoting from #2036:

In LoopBack 3, it was very easy to get a fully-featured CRUD REST API with
very little code: a model definition describing model properties + a model
configuration specifying which datasource to use.

Let's provide the same simplicity to LB4 users too.

  • User creates a model class and uses decorators to define model properties.
    (No change here.)
  • User declaratively defines what kind of data-access patterns to provide
    (CRUD, KeyValue, etc.) and what datasource to use under the hood.
  • @loopback/boot processes this configuration and registers appropriate
    repositories & controllers with the app.

In this Spike, I am demonstrating a PoC implementation of an extensible booter
that processed model configuration files in JSON formats and uses 3rd-party
plugins to build repository & controller classes at runtime.

See SPIKE.md for further details.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

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

👉 Check out how to submit a PR 👈

@bajtos bajtos added spike Boot REST Issues related to @loopback/rest package and REST transport in general labels Aug 29, 2019
@bajtos bajtos requested a review from raymondfeng August 29, 2019 13:44
@bajtos bajtos self-assigned this Aug 29, 2019
packages/context/src/binding-decorator.ts Outdated Show resolved Hide resolved
packages/context/src/inject.ts Outdated Show resolved Hide resolved
packages/context/src/binding-inspector.ts Outdated Show resolved Hide resolved
// Start Application
await app.start();
}

async function givenTodoRepository() {
todoRepo = await app.getRepository(TodoRepository);
todoRepo = await app.get<TodoRepository>('repositories.TodoRepository');
Copy link
Member Author

@bajtos bajtos Aug 29, 2019

Choose a reason for hiding this comment

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

We don't have a repository class we could use here, TodoRepository is just a type alias for EntityCrudRepository<Todo, number>.

import {GeocoderService} from '../../../services';
import {aLocation, givenTodo} from '../../helpers';

describe('TodoController', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I see little value in writing unit tests for controllers contributed by 3rd-party packages. The controller code should have been already tested in that package (@loopback/rest-crud in this case).

import {TodoRepository} from '../repositories';
import {GeocoderService} from '../services';

export class TodoController {
Copy link
Member Author

Choose a reason for hiding this comment

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

No controller files, yay 🎉

import {DefaultCrudRepository, juggler} from '@loopback/repository';
import {Todo, TodoRelations} from '../models';

export class TodoRepository extends DefaultCrudRepository<
Copy link
Member Author

Choose a reason for hiding this comment

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

No repository files, yay 🎉

type: 'string',
})
remindAtGeo?: string; // latitude,longitude

Copy link
Member Author

Choose a reason for hiding this comment

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

In order to implement geocoder integration while using public-models/product.config.json, we need to customize the controller method defined by @loopback/rest-crud. That's kind of against the spirit of this change, where we want to use the default REST API as provided by LB. At least for the initial release, I'd like us to recommend users to switch from @loopback/rest-crud to lb4 repository & lb4 controller if they need any tweaks in the built-in functionality.

@bajtos bajtos force-pushed the spike/crud-rest-booter branch 2 times, most recently from 12103a0 to e116ab5 Compare August 30, 2019 11:36
@bajtos bajtos requested review from a team August 30, 2019 11:38
@bajtos
Copy link
Member Author

bajtos commented Aug 30, 2019

The spike is done and ready for review.

packages/context/src/binding-decorator.ts Outdated Show resolved Hide resolved
packages/context/src/inject.ts Outdated Show resolved Hide resolved
_SPIKE_.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

👍 👍 Great effort. I left a few comments regarding the flexibility to modify the apis defined in controller class.

The todo example looks simple enough to config 👍

_SPIKE_.md Show resolved Hide resolved
_SPIKE_.md Show resolved Hide resolved
_SPIKE_.md Show resolved Hide resolved
_SPIKE_.md Show resolved Hide resolved
@bajtos bajtos force-pushed the spike/crud-rest-booter branch 2 times, most recently from b7c6feb to 67d1128 Compare September 6, 2019 14:27
@bajtos
Copy link
Member Author

bajtos commented Sep 6, 2019

I have updated the spike as follows:

  • Rebased on top of the latest master
  • Reworked the implementation to load model configs from JS files in dist/public-models/*.config.js instead of public-models/*.config.json
  • Moved RestBooter to @loopback/boot and renamed it to ModelApiBooter
  • Improved SPIKE docs based on review comments, updated list of implementation stories

I consider this spike as done and ready for final review & approval.

@strongloop/loopback-next PTAL.

@bajtos bajtos requested review from a team, raymondfeng and jannyHou September 6, 2019 14:36
@bajtos bajtos force-pushed the spike/crud-rest-booter branch from 4f74b98 to 446d5f5 Compare September 10, 2019 09:02
@bajtos
Copy link
Member Author

bajtos commented Sep 10, 2019

Thank you all for feedback. I decided to address it the following way:

  • I have renamed public-models to model-endpoints as suggested in the comments.
  • I am using the naming convention {model-name}.rest-config.ts, this will allow us to introduce e.g. {model-name}.grpc-config.ts in the future.
  • The ModelApiConfig object is referencing the model class directly now, setting model field to an imported class constructor.
  • I have removed app.model() API that's no longer needed.
  • I have update the SPIKE document accordingly.

Let's do a final round of reviews and then call the spike done.

@hacksparrow
Copy link
Contributor

I am not sure if CrudRestApiConfig is the best name (it does not mention "model"), suggestions are welcome.

Since we are focused on REST for now CrudRestApiConfig sound OK to me. And it resides under @loopback/crud-rest.

If we support more flavors, they'll have their own names and reside in their own package, if not in a generic one where CrudRestApiConfig may also reside.

@hacksparrow
Copy link
Contributor

Oh, just realized your other point. I think it is important to give the context to CrudRestApiConfig like mode-endpoints. So maybe ModelCrudRestApiConfig?

Simply CrudRestApiConfig sounds like the LoopBack default, which it's not.

@bajtos
Copy link
Member Author

bajtos commented Sep 12, 2019

Oh, just realized your other point. I think it is important to give the context to CrudRestApiConfig like mode-endpoints. So maybe ModelCrudRestApiConfig?

Simply CrudRestApiConfig sounds like the LoopBack default, which it's not.

Exactly!

I'll update the interface to ModelCrudRestApiConfig then. (It's an awfully long name though 🙈 )

@bajtos
Copy link
Member Author

bajtos commented Sep 12, 2019

It's ok if we don't get the names exactly right in the spike, we can tweak them later during code review of the real implementation.

@bajtos bajtos mentioned this pull request Sep 13, 2019
3 tasks
Signed-off-by: Miroslav Bajtoš <[email protected]>
@bajtos
Copy link
Member Author

bajtos commented Sep 13, 2019

Follow-up stories:

I am closing the spike as done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Boot REST Issues related to @loopback/rest package and REST transport in general spike
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants