Skip to content
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(core): add sugar function for model binding #832

Closed
wants to merge 1 commit into from

Conversation

shimks
Copy link
Contributor

@shimks shimks commented Jan 4, 2018

Description

Adding in sugar function for ctx.bind('model.modelName').toClass(modelCtor).tag('model'). This is done as a part of #786 and #441. In order for metadata for the models to be accessed, users require access to the names of the models, which in turn can be provided by using these bindings.

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide

@shimks
Copy link
Contributor Author

shimks commented Jan 4, 2018

@slnode test please

@@ -41,6 +41,12 @@ export class Application extends Context {
this.controller(ctor);
}
}

if (options.models) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on #742, should we remove options.models support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, i wrote this before the discussion. I'll get rid of this option

@shimks shimks force-pushed the model-binding-sugar branch from f7c3886 to d3aff02 Compare January 4, 2018 19:25
@shimks shimks force-pushed the model-binding-sugar branch from d3aff02 to 9a4b6d8 Compare January 4, 2018 20:35
@bajtos
Copy link
Member

bajtos commented Jan 5, 2018

The code changes look reasonably to me, but I am not sure what is the value of these bindings? So far, our Best Practices don't register models for dependency injection because models and repositories are pretty tightly coupled together.

How do you envision that this model registry will be used in applications? What will the new best practices look like? What kind of changes will be needed in https://github.com/strongloop/loopback4-example-getting-started?

@shimks
Copy link
Contributor Author

shimks commented Jan 5, 2018

@bajtos I'm currently working on #786 which would leverage the @model and @property decorators to build JSON schemas from their metadata. My approach was to register these models with dependency injection at boot in order to get their constructors to use as keys to access the metadata.

Essentially, there would be no changes required in any of the examples (or our best practices) since these are intended to be used at boot as sugar. On second thought, once #786 is done we could think about changing our Best Practices section to reflect some of the changes made. An example might be to drop type as a key to pass in for @property, since our metadata is able to find that out itself.

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.

@shimks

I'm currently working on #786 which would leverage the @model and @Property decorators to build JSON schemas from their metadata. My approach was to register these models with dependency injection at boot in order to get their constructors to use as keys to access the metadata.

I am afraid I still don't understand. Could you please point me to the code where demonstrating this approach? Is it necessary to register those models with DI, cannot we use an approach that's closer to our current Best Practices?

On second thought, once #786 is done we could think about changing our Best Practices section to reflect some of the changes made. An example might be to drop type as a key to pass in for @Property, since our metadata is able to find that out itself.

+1 for dropping type from @property annotations in our Best Practices code examples.

@shimks
Copy link
Contributor Author

shimks commented Jan 15, 2018

@bajtos Looks like I had a couple of misunderstandings of what #786 entailed. This PR is no longer useful to the aforementioned PR and I'd be ok with closing it

@bajtos
Copy link
Member

bajtos commented Jan 16, 2018

This PR is no longer useful to the aforementioned PR and I'd be ok with closing it

Ok, let's close it then.

@bajtos bajtos closed this Jan 16, 2018
@bajtos bajtos deleted the model-binding-sugar branch January 16, 2018 15:03
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.

4 participants