Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

feat(rest): add mountExpressRouter #2643

Merged
merged 1 commit into from
Mar 29, 2019
Merged

feat(rest): add mountExpressRouter #2643

merged 1 commit into from
Mar 29, 2019

Conversation

nabdelgadir
Copy link
Contributor

@nabdelgadir nabdelgadir commented Mar 25, 2019 •
edited
Loading

Closes #2389.

Add mountExpressRouter to RestApplication and RestServer and include in Routes.md.

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 force-pushed the add-mount-express-router branch 2 times, most recently from 82d2b4a to c749427 Compare March 27, 2019 19:25
bajtos
bajtos reviewed Mar 28, 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.

Nice progress.

@nabdelgadir nabdelgadir force-pushed the add-mount-express-router branch 3 times, most recently from ca85204 to 4decf7b Compare March 29, 2019 00:12
@bajtos bajtos added feature REST Issues related to @loopback/rest package and REST transport in general Migration labels Mar 29, 2019
bajtos
bajtos reviewed Mar 29, 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.

Nice!

I see there are few tests that are missing the implementation, other than that I think you are almost done here 👍

Is it perhaps the time to switch from a draft to a regular pull request?

@nabdelgadir nabdelgadir force-pushed the add-mount-express-router branch from 4decf7b to 9a2024e Compare March 29, 2019 14:44
@nabdelgadir nabdelgadir changed the title [WIP] feat(rest): add mountExpressRouter feat(rest): add mountExpressRouter Mar 29, 2019
@nabdelgadir nabdelgadir marked this pull request as ready for review March 29, 2019 14:44
@nabdelgadir nabdelgadir force-pushed the add-mount-express-router branch from 9a2024e to f47ccc5 Compare March 29, 2019 14:47
bajtos
bajtos reviewed Mar 29, 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.

Almost there! Please fix the assertion in the test, other comments can be ignored.

emonddr
emonddr approved these changes Mar 29, 2019
View reviewed changes
Copy link
Contributor

@emonddr emonddr left a comment

Choose a reason for hiding this comment

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

great job!

@nabdelgadir nabdelgadir force-pushed the add-mount-express-router branch from f47ccc5 to a069321 Compare March 29, 2019 15:46
@nabdelgadir nabdelgadir requested a review from bajtos March 29, 2019 15:47
@nabdelgadir nabdelgadir force-pushed the add-mount-express-router branch 2 times, most recently from 7a61cf1 to 93f8841 Compare March 29, 2019 15:50
b-admike
b-admike reviewed Mar 29, 2019
View reviewed changes
packages/rest/src/__tests__/integration/rest.application.integration.ts
});

it('mounts an express Router without spec', async () => {
const router = express.Router();
Copy link
Contributor

Choose a reason for hiding this comment

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

What would the default generated(?) spec look like here if one is not provided? Can we add an assertion for it?

Copy link
Contributor Author

@nabdelgadir nabdelgadir Mar 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The spec is optional, so if it isn't provided then it won't get generated or appear in openapi.json.

packages/rest/src/__tests__/integration/rest.application.integration.ts
res.send('External dog');
});

restApp.static('/dogs', ASSETS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we silently replace the static route in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When it's looking for the route, it first checks if there's an external route, then if it can't find it then it uses the static one (see here), so I believe the static one still exists, but is overshadowed by the external one.

b-admike
b-admike approved these changes Mar 29, 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.

Mostly nitpicks, LGTM overall 💯.

raymondfeng
raymondfeng reviewed Mar 29, 2019
View reviewed changes
@nabdelgadir nabdelgadir force-pushed the add-mount-express-router branch from 93f8841 to 2937121 Compare March 29, 2019 19:40
@nabdelgadir @bajtos
feat(rest): add mountExpressRouter
8714035 
Co-authored-by: Miroslav Bajtoš <mbajtoss@gmail.com>
@nabdelgadir nabdelgadir force-pushed the add-mount-express-router branch from 2937121 to 8714035 Compare March 29, 2019 20:09
@nabdelgadir nabdelgadir merged commit be21cde into master Mar 29, 2019
@nabdelgadir nabdelgadir deleted the add-mount-express-router branch March 29, 2019 20:30
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@raymondfeng raymondfeng raymondfeng left review comments

@jannyHou jannyHou jannyHou left review comments

@emonddr emonddr emonddr approved these changes

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

@bajtos bajtos Awaiting requested review from bajtos

Assignees
No one assigned
Labels
feature Migration REST Issues related to @loopback/rest package and REST transport in general
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nabdelgadir @raymondfeng @bajtos @emonddr @jannyHou @b-admike