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

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

Merged
merged 1 commit into from
Sep 20, 2019

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng 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 jannyHou left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@jannyHou
Copy link
Contributor

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

@raymondfeng
Copy link
Contributor Author

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>,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 September 16, 2019 16:25
@raymondfeng
Copy link
Contributor Author

@bajtos PTAL

@bajtos
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
Copy link
Contributor Author

@bajtos Please approve loopbackio/loopback-datasource-juggler#1778 first.

raymondfeng added a commit to loopbackio/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 Compare September 19, 2019 22:19
@raymondfeng raymondfeng merged commit 0094ded into master Sep 20, 2019
@raymondfeng raymondfeng deleted the fix-3671 branch September 20, 2019 16:27
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.

Unable to create Recursive Model (Non Entity)
3 participants