-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat(boot): add helpers to create a booter for component applications #5304
Conversation
14fa1bb
to
38e91de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* Add bindings from the component application to the main application | ||
*/ | ||
const bindings = componentApp.find(binding => { | ||
return filter(binding) && !this.mainApp.contains(binding.key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question for !this.mainApp.contains(binding.key)
:
what if the sub app wants to override the main app's bindings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are certain bindings from the sub-app should not override the main app. We should carefully review such bindings - for example, CoreBindings.*
, RestBindings.*
. Maybe one way to solve it is to mark such bindings locked. If so, we can update this code to only skip locked bindings in the main app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should carefully review such bindings - for example,
CoreBindings.*
,RestBindings.*
True, agreed. I came up with this question due to the chat happened on public slack this morning, I think that community user tries to contribute custom artifacts(like models, in his case) from the component.
Maybe one way to solve it is to mark such bindings locked. If so, we can update this code to only skip locked bindings in the main app.
I feel it's a case by case preference, maybe we can provide a default filter to skip those core bindings. And if user really wants to override, they provide custom filters :)
(don't have to address it in this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I improved the binding filtering as follows:
- Bindings that exist in the component application before
boot
are skipped. - Locked bindings in the main application will not be overridden.
- Other bindings are selected by the binding filter. By default, all bindings are picked.
38e91de
to
f5a6851
Compare
Use case: - Create a LoopBack application - Add controllers, services, datasources - Now use this application as a component with the main applicaiton - The main application needs to have artifacts from the component application
f5a6851
to
e64ad10
Compare
Documentation for sub application binding in main application doesn't contain any exmaple. That will be helpful |
|
||
// This can be done in the constructor of `MainApp` too. Make sure the component | ||
// is registered before calling `app.boot()`. | ||
mainApp.component(SubAppComponent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raymondfeng I find these instructions quite involved. Would the following approach work & make sense too?
class MyApp extends BootMixin(Application) {
constructor(options: ApplicationOptions) {
super(options);
this.add(
createComponentApplicationBooterBinding(new SubApp()),
);
}
}
I think it would be great to add a helper method to BootMixin
to wrap those two calls in a single API, but I find it difficult to find a good method name. How about addApplicationBooter
?
Intended usage:
class MyApp extends BootMixin(Application) {
constructor(options: ApplicationOptions) {
super(options);
this.addApplicationBooter(new SubApp());
}
}
WDYT?
Use case:
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈