refactor: simplify app setup via RestApplication (part II) #968
Conversation
bajtos
changed the title
There was a problem hiding this comment.
Thanks for the help in finishing my task!
Aside from the comments I've made below, the package example-hello-world also has an unnecessary import (Application) that needs to be deleted.
Otherwise, LGTM
EDIT: the README from example-log-extension can also use RestApplication to simplify some process
| } from '@loopback/authentication'; | ||
| import {RestComponent, RestServer} from '@loopback/rest'; | ||
| import {MyAuthStrategyProvider} from './providers/auth-strategy'; | ||
| import {MyController} from './controllers/my-controller'; |
There was a problem hiding this comment.
at lines 148 and 153, Application and RestComponent aren't needed anymore
| // This file is licensed under the MIT License. | ||
| // License text available at https://opensource.org/licenses/MIT | ||
|
|
||
| import {Application, ApplicationConfig} from '@loopback/core'; |
There was a problem hiding this comment.
No need to import Application anymore
packages/rest/README.md
Outdated
| `@loopback/rest`: | ||
|
|
||
| ```ts | ||
| import {Application} from '@loopback/core'; |
There was a problem hiding this comment.
I don't think we need to install @loopback/core as a dependency anymore
There was a problem hiding this comment.
This might be one of those cases where the import is required for the compiler to make sense of another required type's dependencies. The easiest way would be to attempt to compile it without this line and find out!
There was a problem hiding this comment.
It's definitely an unused import. I've used RestApplication without installing / using @loopback/core.
There was a problem hiding this comment.
Interesting. I copied this example into a standalone file and compiled it using the following command:
tsc x.ts --target es2017 -m CommonJS
However, I received a bunch of errors about missing type information for Node.js. To fix that, I had to add @types/node to regular dependencies of @loopback/rest. With that change in place, the example compiles fine.
- Move any REST-related setup from app.start() back to app constructor. - Update the project template to use RestApplication too - Update authentication README to use RestApplication
bajtos
force-pushed
the
refactor/simplify-app-setup
branch
from
28645ac to
a48b72d
Compare
bajtos
force-pushed
the
refactor/simplify-app-setup
branch
from
a48b72d to
c1ad7cb
Compare
|
Thank you for the feedback. I have rebase this patch on top of the latest master and resolved your comments in c1ad7cb. @kjdelisle @shimks @virkt25 LGTY now? |
This is a follow-up for #955, a part of #846
Checklist
npm testpasses on your machinepackages/cliwere updatedpackages/example-*were updated