Skip to content

Conversation

@bajtos
Copy link
Member

@bajtos bajtos commented Feb 14, 2018

Enhance RestApplication class to provide shortcuts for registering routes and configuring the master OpenAPI spec.

See #846 and #955.

Checklist

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

});

describe('RestApplication', () => {
it('supports function-based routes', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a copy of an existing test which is using server.route.

@bajtos bajtos force-pushed the feat/rest-app-route branch from 6271283 to 675ab0e Compare February 14, 2018 10:22
@bajtos
Copy link
Member Author

bajtos commented Feb 14, 2018

The tests are a copy and paste of existing tests using server.* instead of app.* APIs.

@bajtos bajtos requested a review from jannyHou February 14, 2018 10:23
Copy link
Contributor

@shimks shimks left a comment

Choose a reason for hiding this comment

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

Thanks for essentially finishing my task here 😆 . The PR looks good to me so far, but I've noticed that with RestApplication, tests were missing for sequence and handler functions. Could we also add in tests for those as part of the PR?

@raymondfeng
Copy link
Contributor

Travis fails with the following error:

1) Routing
       RestApplication
         supports controller routes declared via app.api():
     Error: expected 200 "OK", got 404 "Not Found"
      at Test._assertStatus (packages/testlab/node_modules/supertest/lib/test.js:266:12)
      at Test._assertFunction (packages/testlab/node_modules/supertest/lib/test.js:281:11)
      at Test.assert (packages/testlab/node_modules/supertest/lib/test.js:171:18)
      at Server.assert (packages/testlab/node_modules/supertest/lib/test.js:131:12)
      at emitCloseNT (net.js:1655:8)
      at _combinedTickCallback (internal/process/next_tick.js:135:11)
      at process._tickCallback (internal/process/next_tick.js:180:9)

Please fix.

@bajtos
Copy link
Member Author

bajtos commented Feb 15, 2018

tests were missing for sequence and handler functions.

app.handler is tested here:
https://github.com/strongloop/loopback-next/blob/baf9928613ccde82c02abbc8249f4163e6a5570b/packages/rest/test/acceptance/bootstrapping/rest.acceptance.ts#L55-L60

I have added a new test for app.sequence, see e923d87.

Now I need to figure out why the tests are failing.

@bajtos
Copy link
Member Author

bajtos commented Feb 15, 2018

It turns out restApp.sequence was broken, I fixed it in 7cc0777 (feel free to review).

What's remaining: fix restApp.api() test(s).

@bajtos
Copy link
Member Author

bajtos commented Feb 15, 2018

All should be good now.

@raymondfeng @shimks @kjdelisle please review

Thanks for essentially finishing my task here

@shimks I think we need to update any examples in our documentation on loopback.io to use these new APIs. I'll post a comment in the issue.

@bajtos bajtos requested a review from shimks February 15, 2018 16:04
Copy link
Contributor

@kjdelisle kjdelisle left a comment

Choose a reason for hiding this comment

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

👍

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.

👍

Enhance `RestApplication` class to provide shortcuts for registering
routes and configuring the master OpenAPI spec.

Fix restApp.sequence() - modify RestServer to inherit the sequence
binding from the application, modify RestComponent to register the
DefaultSequence at the app level (instead of RestServer level).

Similarly, modify RestServer to inherit the API_SPEC binding from
the application, modify RestComponent to register an empty spec at
the app level (instead of RestServer level).
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.

7 participants