-
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
[Boot] Model Booter #1428
Comments
I think it makes sense to include this in DP3, though we're a bit tight on time. |
@virkt25 @dhmlau I am little bit confused why we need a model booter. AFAIK, none of our code is binding Model classes to context. In LB4, a Model is describing data interface and as such it's usually not injected via DI. Persistence behavior is provided by model repositories, and that's the entity we need to register in the context for dependency injection. I am proposing to close this story as not relevant (for now). Thoughts? /cc @raymondfeng |
At the moment, we need a reference to a model class at compile time to tell TypeScript what shape the arguments/return values have. In theory, it is possible to inject different implementation of model classes, but our current Repository design does not really support that IIRC. See e.g. export class TodoRepository extends DefaultCrudRepository<
Todo,
typeof Todo.prototype.id
> {
constructor(
@inject('datasources.db') protected datasource: juggler.DataSource,
) {
super(Todo, datasource);
}
} I suppose we could change the repository to allow custom export class TodoRepository extends DefaultCrudRepository<
Todo,
typeof Todo.prototype.id
> {
constructor(
@inject('models.Todo') protected todoModel: Class<Todo>,
@inject('datasources.db') protected datasource: juggler.DataSource,
) {
super(todoModel, datasource);
}
} I am not convinced it's a useful feature - why and how would we want to provide a different model implementation? In my mind, one of the primary use cases for Dependency Injection is to allow tests to be written. When it comes to models, there was no need for mocking/stubbing model classes so far, see http://loopback.io/doc/en/lb4/Testing-your-application.html In that light, and considering that making models injectable requires more changes than a new booter, I think this story is out of scope of DP3 and most likely out of scope of 4.0 GA too. |
@bajtos and I had a separate discussion, and I'm good with leaving out the model booter. |
I'm ok with leaving out the story. For future when we do come back to this (if / ever) ... I would like the syntax for declaring a Repository to be simplified to not require the Model in |
Without the providing the specific Model class as a specialization of the generic Consider the following example: const product = productRepository.findById(1);
product.foo = 'bar'; Right now, TypeScript understand that If the I suppose we could parameterize individual methods, e.g. const user = productRepository.findById<User>(1); What can be done: split the model into an interface declaring properties & methods and the actual model class. Use the model interface to create a specialized version of Repository classes. I am not sure if the extra complexity and duplication is worth the benefits though. |
Oh I'm definitely not in favour of parameterizing individual methods! I had a feeling it won't be possible because we need it for typing information ... was just curious to see if there was perhaps a way around that I wasn't aware of. As much as I dislike the syntax, the benefit of having the type information is much more! I don't think we need to add complexity / duplication to resolve this at all -- time and effort are better spent elsewhere. |
Talked to @raymondfeng and here's the summary of our discussion:
|
Fair enough 👍 I feel this does not belong to 4.0 GA scope as we have defined it, I am relabeling this story as post-GA. @raymondfeng @virkt25 please let me know if you disagree. |
This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the |
This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the |
Description / Steps to reproduce / Feature proposal
Once #1221 is done OR in parallel to that PR, a booter needs to be created to boot
.model.js
files using@loopback/boot
.Current Behavior
Expected Behavior
Acceptance Criteria
model
artifact type. Similar to other artifacts (controllers, datasources, repositories, etc).generated
folder which will havebase
classes for the models for typing purposes that will be extended by the actualmodel
classes. We probably don't need to pollute the Context by binding the base classes to the Context -- so need to determine a way around this.See Reporting Issues for more tips on writing good issues
The text was updated successfully, but these errors were encountered: