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(docs): rename legacy juggler to juggler #6037

Merged
merged 2 commits into from
Aug 5, 2020
Merged

Conversation

nabdelgadir
Copy link
Contributor

Fixes #5558

I also renamed it in the CLI.

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 👈

As mentioned before, we are using the legacy juggler bridge as a temporary
solution. This may change in the future since the Juggler is pretty.. you know,
legacy. However, it's important to realize how LB4 deals with models.
As mentioned before, we are using the juggler bridge as a temporary solution.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll need to rephrase this or even remove it altogether. @raymondfeng @bajtos thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

+1 in removing this paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks!

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.

Thank you @nabdelgadir for the pull request. The changes look mostly good 👏🏻

I think we should take this opportunity and remove the experimental status, see my comments below.

@@ -6,8 +6,7 @@ This module provides a common set of interfaces for interacting with databases.

**NOTE**: This module is experimental and evolving. It is likely going to be
refactored and decomposed into multiple modules as we refine the story based on
the legacy `loopback-datasource-juggler` and connector modules from LoopBack
3.x.
`loopback-datasource-juggler` and connector modules from LoopBack 3.x.
Copy link
Member

Choose a reason for hiding this comment

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

I think @loopback/repository is no longer experimental. It's true that we are thinking about decompose this module into multiple smaller ones, but that's a very long term goal, far in the future.

I am proposing to remove this paragraph entirely.

@@ -9,8 +9,7 @@
* @remarks
* *NOTE:* This module is experimental and evolving. It is likely going to be
* refactored and decomposed into multiple modules as we refine the story based
* on the legacy loopback-datasource-juggler and connector modules from LoopBack
* 3.x.
* on the loopback-datasource-juggler and connector modules from LoopBack 3.x.
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, let's remove the "experimental status" note.

Copy link
Contributor

@agnes512 agnes512 left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@nabdelgadir nabdelgadir merged commit 921e6ae into master Aug 5, 2020
@nabdelgadir nabdelgadir deleted the rename-juggler branch August 5, 2020 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename "legacy juggler bridge" mentioned in docs
5 participants