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

find* methods include navigational properties #2960

Merged
merged 1 commit into from
Jun 1, 2019
Merged

Conversation

nabdelgadir
Copy link
Contributor

@nabdelgadir nabdelgadir commented May 24, 2019

Resolves #2632.

  • find and findById now use T & Relations instead of T
  • New prompt for lb4 model that asks for relations in order to create the relation interface (e.g. export interface TodoRelations {}). It leaves it empty if there are no current relations.
  • Newly scaffolded models from lb4 model include {modelName}WithRelations type (e.g. export type TodoWithRelations = Todo & TodoRelations;)
  • Generic parameter Relations added to DefaultCrudRepository
  • When scaffolding a repository with lb4 repository, it now includes {modelName}Relations (e.g.
export class TodoListRepository extends DefaultCrudRepository<
  TodoList,
  typeof TodoList.prototype.id
  TodoListRelations
> 

)

Question: should all models include a relations interface? If so, I'll update the examples/docs.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

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

👉 Check out how to submit a PR 👈

@nabdelgadir nabdelgadir self-assigned this May 24, 2019
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.

Great start!

I feel the changes in CLI are going in wrong direction and adding unnecessary complexity, let's discuss.

packages/cli/generators/model/index.js Outdated Show resolved Hide resolved
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.

Almost there!

In the new commits, I added empty interfaces because I think they should be populated only after #2633 is landed. What do you think?

I am not sure if I understand your question correctly.

IMO, when a model already has some relations defined, then the *Relations interface should have the navigational properties defined too. We will need these navigational properties for our next steps on the journey to implement the feature "inclusion of related models".

As I see it, #2633 is about automating the task of adding navigational properties. Until it's done, developers will have to define these properties manually, which is exactly what I am asking you to do in this pull request too.

examples/todo-list/src/models/todo-list-image.model.ts Outdated Show resolved Hide resolved
examples/todo-list/src/models/todo-list.model.ts Outdated Show resolved Hide resolved
examples/todo-list/src/models/todo.model.ts Outdated Show resolved Hide resolved
examples/todo/src/models/todo.model.ts Outdated Show resolved Hide resolved
packages/cli/generators/model/index.js Outdated Show resolved Hide resolved
packages/cli/generators/model/templates/model.ts.ejs Outdated Show resolved Hide resolved
@nabdelgadir nabdelgadir force-pushed the nav-properties branch 2 times, most recently from bb2703c to 35c43d8 Compare May 30, 2019 22:32
@nabdelgadir
Copy link
Contributor Author

@bajtos

IMO, when a model already has some relations defined, then the *Relations interface should have the navigational properties defined too. We will need these navigational properties for our next steps on the journey to implement the feature "inclusion of related models".

As I see it, #2633 is about automating the task of adding navigational properties. Until it's done, developers will have to define these properties manually, which is exactly what I am asking you to do in this pull request too.

I thought the purpose of #2633 was only to add them to the TodoList example and verify it works for that example (and then after it's tested there, then we could add the properties of the *Relations interface to the other examples/docs). But I guess adding them here makes more sense, so now they're a part of this PR 👍

Also, did you mean #2988 was for automating instead of #2633?

@nabdelgadir nabdelgadir force-pushed the nav-properties branch 2 times, most recently from 38e42e4 to 5d5450e Compare May 30, 2019 22:45
@bajtos
Copy link
Member

bajtos commented May 31, 2019

Also, did you mean #2988 was for automating instead of #2633?

Yes, I meant #2988. Sorry for the confusion!

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.

LGTM 👏

docs/site/BelongsTo-relation.md Show resolved Hide resolved
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

Nice 👏 LGTM.

@nabdelgadir nabdelgadir force-pushed the nav-properties branch 4 times, most recently from 2ddd367 to 10bea99 Compare May 31, 2019 21:11
Updated find* methods to include navigational properties, introduced *Relations to models, and updated DefaultCrudRepository to include Relations

Co-authored-by: Miroslav Bajtoš <[email protected]>
@nabdelgadir nabdelgadir merged commit 1f0aa0b into master Jun 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the nav-properties branch June 1, 2019 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI feature Relations Model relations (has many, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify Repository find* methods to include navigational properties
3 participants