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.

Sign up for GitHub

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

Jump to bottom

feat: add booter-lb3app package #2682

Merged
merged 1 commit into from
Apr 24, 2019
Merged

feat: add booter-lb3app package #2682

merged 1 commit into from
Apr 24, 2019

Conversation

nabdelgadir
Copy link
Contributor

@nabdelgadir nabdelgadir commented Apr 3, 2019 •
edited
Loading

Closes #2391.

This PR adds the booter-lb3app package that assists in booting and mounting a LoopBack 3 application and exposing its REST API through LoopBack 4.

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

@nabdelgadir nabdelgadir changed the title feat: add booter-lb3app package [WIP] feat: add booter-lb3app package Apr 3, 2019
@nabdelgadir nabdelgadir self-assigned this Apr 4, 2019
@nabdelgadir nabdelgadir force-pushed the lb3-booter branch 4 times, most recently from f21ec34 to a697d00 Compare April 11, 2019 17:56
@nabdelgadir nabdelgadir force-pushed the lb3-booter branch 2 times, most recently from c69ed7d to 86e416b Compare April 12, 2019 17:15
@nabdelgadir nabdelgadir marked this pull request as ready for review April 12, 2019 17:24
@nabdelgadir nabdelgadir force-pushed the lb3-booter branch 2 times, most recently from 8951de5 to 4d2dd1e Compare April 12, 2019 20:09
@nabdelgadir nabdelgadir changed the title [WIP] feat: add booter-lb3app package feat: add booter-lb3app package Apr 15, 2019
bajtos
bajtos suggested changes Apr 15, 2019
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

The implementations looks pretty good. I see that you have followed the checklist for adding new packages, kudos for that 👏

Let's improve the test suite now. I find the current test suite as too difficult to reason about. There are many fixture files adding lot of functionality on top of default LB3 & LB4 applications, it's not clear what is relevant for the LB3 booter and what's just "random stuff copied from elsewhere".

Ideally, I'd like the test suite to be more focused. Start with a minimal LB3 application with a single model, place this e.g. to /fixtures/minimal-app. In your test code, create a minimal LB4 application. I think you don't even need to create any new class, just call new RestApplication. Configure this LB4 app with the new booter and start it. Then verify that the endpoints from LB3 were exposed as expected. I think that's good enough for the first version of this feature. We can add more tests later to verify more complex configuration, if we ever discover any problems there.

packages/booter-lb3app/package.json Outdated Show resolved Hide resolved
packages/booter-lb3app/package.json Outdated Show resolved Hide resolved
packages/booter-lb3app/package.json Outdated Show resolved Hide resolved
packages/booter-lb3app/src/__tests__/acceptance/booter-lb3app.acceptance.ts Outdated Show resolved Hide resolved
packages/booter-lb3app/src/__tests__/fixtures/legacy/common/models/coffee-shop.js Outdated Show resolved Hide resolved
packages/booter-lb3app/src/__tests__/fixtures/src/application.ts Outdated Show resolved Hide resolved
packages/booter-lb3app/src/__tests__/fixtures/src/coffee.controller.ts Outdated Show resolved Hide resolved
packages/booter-lb3app/src/__tests__/fixtures/src/datasources/ds.datasource.json Outdated Show resolved Hide resolved
packages/booter-lb3app/src/__tests__/fixtures/src/sequence.ts Outdated Show resolved Hide resolved
packages/booter-lb3app/src/lb3app.booter.ts Outdated Show resolved Hide resolved
@nabdelgadir nabdelgadir requested a review from bajtos April 17, 2019 23:25
@nabdelgadir nabdelgadir force-pushed the lb3-booter branch 3 times, most recently from a28a88f to 465743e Compare April 22, 2019 13:27
b-admike
b-admike reviewed Apr 22, 2019
View reviewed changes
Copy link
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.

I've went through most of the commit (minus some updating deps), and overall the patch looks great 👍.

packages/booter-lb3app/src/__tests__/acceptance/booter-lb3app.acceptance.ts Outdated Show resolved Hide resolved
docs/site/MONOREPO.md Outdated Show resolved Hide resolved
greenkeeper.json Outdated Show resolved Hide resolved
packages/booter-lb3app/src/__tests__/acceptance/booter-lb3app.acceptance.ts Outdated
@@ -59,6 +59,28 @@ describe('booter-lb3app', () => {
});
});

context('open API spec endpoints', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

packages/booter-lb3app/src/__tests__/acceptance/booter-lb3app.acceptance.ts
@@ -79,12 +68,55 @@ describe('booter-lb3app', () => {
'/api/CoffeeShops/count',
]);
});

context('LoopBack 3 authentication', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test coverage 👍

@nabdelgadir nabdelgadir force-pushed the lb3-booter branch 2 times, most recently from 444b49a to 70de24c Compare April 22, 2019 17:09
b-admike
b-admike approved these changes Apr 23, 2019
View reviewed changes
Copy link
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.

LGTM

@nabdelgadir nabdelgadir force-pushed the lb3-booter branch 3 times, most recently from c122fd5 to 694daf8 Compare April 24, 2019 13:14
@nabdelgadir @bajtos
feat: add booter-lb3app package
898fac3 
Co-authored-by: Miroslav Bajtoš <[email protected]>
@dhmlau dhmlau mentioned this pull request Apr 24, 2019
Closed
@nabdelgadir nabdelgadir merged commit 993a97f into master Apr 24, 2019
@nabdelgadir nabdelgadir deleted the lb3-booter branch April 24, 2019 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raymondfeng raymondfeng raymondfeng approved these changes

@b-admike b-admike b-admike approved these changes

@hacksparrow hacksparrow Awaiting requested review from hacksparrow

@jannyHou jannyHou Awaiting requested review from jannyHou

@bajtos bajtos Awaiting requested review from bajtos

Assignees

@nabdelgadir nabdelgadir

Labels
Boot feature Migration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Boot and mount LB3 app
4 participants
@nabdelgadir @raymondfeng @bajtos @b-admike