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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(repository): allow model classes with recursive type references #3722

Merged
merged 1 commit into from Sep 20, 2019

Conversation

@raymondfeng
Copy link
Member

raymondfeng commented Sep 12, 2019

Fixes #3671 (comment)

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 馃憟

Copy link
Contributor

jannyHou left a comment

馃憤 LGTM

@jannyHou

This comment has been minimized.

Copy link
Contributor

jannyHou commented Sep 12, 2019

Some code change is probably not compatible with mongodb connector: https://travis-ci.com/strongloop/loopback-next/jobs/234512187

@raymondfeng raymondfeng force-pushed the fix-3671 branch from 14ac1ee to bb2254f Sep 12, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Sep 12, 2019

Some code change is probably not compatible with mongodb connector: https://travis-ci.com/strongloop/loopback-next/jobs/234512187

Fixed.

}

// Create an internal legacy Model attached to the datasource
private definePersistedModel(
entityClass: typeof Model,
visited: Map<typeof Model, typeof juggler.PersistedModel>,

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 13, 2019

Member

Can we leverage DataSource's ModelBuilder instance that's already keeping track of models that have been defined, instead of creating a new map?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Sep 13, 2019

Author Member

Probably not as ModelBuilder has no idea of LB4 entity class.

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 19, 2019

Member

ModelBuilder is keeping track of all models that are backing LB4 entity classes. Whenever we create a new Repository for an LB4 entity class, a new (Persisted)Model is registered with the ModelBuilder behind the datasource.

@raymondfeng raymondfeng requested a review from bajtos Sep 16, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Sep 16, 2019

@bajtos PTAL

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Sep 19, 2019

I am not very happy about the current implementation, because it introduces multiple places that are keeping track of visited models (visited, dataSource.modelBuilder.models).

Juggler already has means for handling circular references in models. I simplified your solution to leverage the concept of "unresolved models", now the fix consists of one new line added.

@raymondfeng PTAL.

If you agree with my proposed direction, then I am proposing the following next steps:

  1. Fix juggler typings to include forceCreate parameter for dataSource.getModel(). See the source.
  2. Publish a new juggler version.
  3. In this patch: remove the cast to any, squash the commits. Please preserve my co-authorship via Co-authored-by trailer, see GitHub docs.
raymondfeng added a commit to strongloop/loopback-datasource-juggler that referenced this pull request Sep 19, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Sep 19, 2019

raymondfeng added a commit to strongloop/loopback-datasource-juggler that referenced this pull request Sep 19, 2019
@raymondfeng raymondfeng force-pushed the fix-3671 branch 3 times, most recently from 212876f to c6cb019 Sep 19, 2019
Fixes #3671 (comment)


Co-authored-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
@raymondfeng raymondfeng force-pushed the fix-3671 branch from c6cb019 to ed6f243 Sep 20, 2019
@raymondfeng raymondfeng merged commit 0094ded into master Sep 20, 2019
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.0006%) to 91.616%
Details
@raymondfeng raymondfeng deleted the fix-3671 branch Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.