Refactor extensions to move framework modules to peerDependencies #5959

bajtos · 2020-07-17T13:36:28Z

As discussed several times in the past (most recently in #5927 (comment)), extensions should use framework modules from the target application via peerDependencies. This pull request makes that change happen.

I split the changes into three commits:

Document the best practice in "Creating components"
Update existing packages (breaking change 💥)
Update CLI template

/cc @strongloop/loopback-maintainers

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 👈

bajtos · 2020-07-17T13:51:45Z

packages/boot/package.json

+  "peerDependencies": {
+    "@loopback/core": "^2.9.1",
+    "@loopback/repository": "^2.9.0",
+    "@loopback/service-proxy": "^2.3.4"


This is potentially tricky.

Before my change, it was possible to use @loopback/boot in applications that don't have @loopback/service-proxy in their dependencies. @loopback/boot would install it's own copy that would be pretty much ignored at runtime.

With the change in place, apps must install both @loopback/repository and @loopback/service-proxy to be able to use @loopback/boot. I wish there was something like optionalPeerDependencies!

I see this as a sign that @loopback/boot is providing too many features and should be split into smaller packages.

It defines contract for Booter implementations and few base classes to help with implementation.

It implements the bootstrapping algorithm (BootMixin).

It provides booters for common artifacts (controllers, repositories, services, etc.).

I think the following structure would be better:

A new module to define Booter contract and possibly base classes. Prior art: https://github.com/strongloop/loopback-next/tree/master/packages/model-api-builder

@loopback/boot should provide BootMixin and artifact mixins that can be loaded using @loopback/core only.

Artifact booters requiring optional extensions (repository booter, service booter, etc.) should be moved either to those extensions (@loopback/repository, @loopback/service-proxy) or to a new package. A question to consider: where to put datasource booter that's shared by repositories and service proxies?

Such change is out of scope of this pull request though. The question is how to address the issue above inside this pull request:

Are we ok to require apps to install both repository and service-proxy if they want to use @loopback/boot? By default, lb4 app includes both, so it may be ok. But then it feels a kind of lame to force users to install packages they don't need.

Should I revert the repository & service-proxy part of this pull request and keep those two packages as regular dependencies?

We can also investigate how to rework booters to not require direct dependency on repository & service-proxy packages. They mostly call application methods mixed in by the user (e.g. app.repository) and import only type definitions for internal consumption from repository/service-proxy, so maybe it's ok to declare them in devDependencies only? The only exception I found is ModelMetadataHelper.getModelMetadata, it's called by ModelBooter to check if a class is decorated with @model().

That's my intention for #5618.

To move this pull request forward, I am going to pick the option 2:

revert the repository & service-proxy part of this pull request and keep those two packages as regular dependencies

It's not ideal, but it will give us more time to consider different options and implement a proper long-term fix.

extensions/authentication-jwt/package.json

bajtos · 2020-07-17T14:05:06Z

Added two more commits to reorganize order of package.json fields and move peerDependencies before regular and dev dependencies, I feel the information about required peer deps is important when reviewing package metadata and therefore should be specified early on.

Signed-off-by: Miroslav Bajtoš <[email protected]>

BREAKING CHANGE: Extensions no longer install framework packages as their own dependencies, they use the framework packages provided by the target application instead. If you are getting `npm install` errors after upgrade, then make sure your project lists all dependencies required by the extensions you are using. Signed-off-by: Miroslav Bajtoš <[email protected]>

Signed-off-by: Miroslav Bajtoš <[email protected]>

bajtos · 2020-07-20T14:47:32Z

@raymondfeng updated per #5959 (comment), LGTY now?

raymondfeng · 2020-07-20T16:02:31Z

docs/site/Creating-components.md

+
+We recommend to use `peerDependencies` to specify what packages is your
+extension expecting in the target application and `devDependencies` to make
+these packages available for extension tests.


We should mention that npm 6 does not install peerDependencies. As a workaround, we need to add duplicate entries in devDependencies so that they can be installed and used for testing.

raymondfeng · 2020-07-20T16:20:08Z

docs/site/Creating-components.md

+```json
+{
+  "peerDependencies": {
+    "@loopback/core": "^1.9.0 || ^2.0.0"


Can lerna or our build update such version ranges for new releases?

See the discussions in #4937 and your pull request #4972.

Quoting from #4972 (comment):

Let's be conservative to use the ^<pkgVersion> for peer dependencies as a release is cut.

The current implementation:

https://github.com/strongloop/loopback-next/blob/6e916494f1bcbdf5ba5ecd20ac8cbadcbae4fc9f/bin/update-peer-deps.js#L38-L41

raymondfeng · 2020-07-20T16:21:58Z

extensions/authentication-jwt/package.json

    "@loopback/authentication": "^4.2.9",
    "@loopback/core": "^2.9.1",
    "@loopback/rest": "^5.2.0",
+    "@loopback/service-proxy": "^2.3.4"


Should @loopback/core stay as a regular dependency?

raymondfeng · 2020-07-20T16:25:42Z

extensions/apiconnect/package.json

    "@loopback/core": "^2.9.1",
-    "@loopback/rest": "^5.2.0",
+    "@loopback/rest": "^5.2.0"


Should @loopback/core stay as a regular dependency? We need to clarify the criteria between dependencies and peerDependencies. I'm thinking about the following:

If the dependency needs to be consistent with the application level, it's a peer dependency (always use the one from the application level)

Otherwise, it should be a regular dependency (and possibly with a different version than the application level)

Another criteria:

If the dependency is the target extension point for this package to contribute extensions, it's a peer dependency.

Using this rule, @loopback/core should be a regular dependency while @loopback/rest should be a peer dependency.

If the dependency needs to be consistent with the application level, it's a peer dependency (always use the one from the application level)

If the dependency is the target extension point for this package to contribute extensions, it's a peer dependency.

Otherwise, it should be a regular dependency (and possibly with a different version than the application level)

Your criteria make sense to me, however I have a different view on how to interpret them.

In my opinion, there should be only one version of @loopback/core installed in any application. Is see the core packages as a cornerstone, a "LoopBack runtime". Among other things, it provides foundation for decorators and manipulation with metadata (MetadataInspector, ParameterDecoratorFactory, etc.) I am concerned that allowing multiple versions of @loopback/core can introduce subtle bugs that will be difficult to troubleshoot, for example when a decorator uses different code for setting metadata than is used by the application to look up that metadata.

Also, if the application developer decides to use their forked version of @loopback/core (e.g. to fix a bug), then I want extensions to use the fixed version of core, not the vanilla version from npm.

I would also like to add one more criteria:

If the dependency is typically installed in target applications, and the extension can support multiple major versions of that dependency at the same time, then the dependency should be a peer dependency.

The use case to consider: where a single version of an extension (e.g. @loopback/extension-health) wants to support multiple version of the framework, so that the maintainers can have a single branch and don't have to maintain different branches (major-version release lines) for each framework version. Let's say the extension depends on @loopback/core@2 and the application depends on @loopback/core@1. This is very reasonable, since the only breaking change listed in @loopback/[email protected] changelog is dropped support for Node.js 8.x. When such extension is installed into the target app, we end up with two copies of @loopback/core, where the extension uses a different "LoopBack runtime" than the target application it's installed to.

Let's discuss!

That's a valid use case that one extension package can support multiple versions of the LoopBack runtime. But it's hard to ensure that with our CI and release process though.

In addition to the containing application, we probably should also take into consideration that certain LoopBack packages can be used standalone outside a scaffolded LB4 application.

That's a valid use case that one extension package can support multiple versions of the LoopBack runtime. But it's hard to ensure that with our CI and release process though.

I now. Personally, I am fine to keep the support for older major versions on a "best effort" basis with no CI check for now. We can setup a more comprehensive CI solution once there are complaints from users about incompatibilities.

In addition to the containing application, we probably should also take into consideration that certain LoopBack packages can be used standalone outside a scaffolded LB4 application.

That's a good argument. IIRC, packages that can be used standalone outside of LB4 apps are usually more low level (@loopback/metadata, @loopback/context etc.) and don't participate in peer dependencies, because they are accessed via higher-level wrappers (@loopback/core).

If a LB package is used by a LB4 application that's not scaffolded via lb4 app, then I believe the app is still going to depend on @loopback/core, therefore my arguments above still apply.

Are you envisioning a project where the developer wants to use an extension depending on @loobpack/core but the application does not depend on @loopback/core? Would that work at all?

Are you envisioning a project where the developer wants to use an extension depending on @loobpack/core but the application does not depend on @loopback/core? Would that work at all?

No. At minimum, we'll use @loopback/core.

No. At minimum, we'll use @loopback/core.

In that case, I think it's reasonable to for extensions to declare @loopback/core as a peer dependency, right?

@raymondfeng Do you agree to keep the currently proposed direction? May I proceed to rebase this PR on top of latest master (there are going to be many merge conflicts to resolve, so I don't want to do it too many times) and get this PR landed soon?

bajtos · 2020-09-01T13:12:40Z

I'll open a series of smaller PRs to land these changes in a way that will minimize merge conflicts.

The first one: #6243

bajtos added refactor breaking-change Extensions labels Jul 17, 2020

bajtos requested review from raymondfeng, hacksparrow and jannyHou July 17, 2020 13:36

bajtos requested review from agnes512, deepakrkris and emonddr as code owners July 17, 2020 13:36

bajtos self-assigned this Jul 17, 2020

bajtos commented Jul 17, 2020

View reviewed changes

extensions/authentication-jwt/package.json Show resolved Hide resolved

bajtos added 2 commits July 20, 2020 16:31

docs: how to work with dependencies in extensions

d90ba54

Signed-off-by: Miroslav Bajtoš <[email protected]>

bajtos force-pushed the refactor/extension-dependencies branch from 82ded8e to 2d9cfec Compare July 20, 2020 14:32

bajtos requested a review from dhmlau as a code owner July 20, 2020 14:32

feat(cli): move "@loopback/core" to peerDependencies for new extensions

3392a1e

Signed-off-by: Miroslav Bajtoš <[email protected]>

bajtos force-pushed the refactor/extension-dependencies branch from 2d9cfec to 3392a1e Compare July 20, 2020 14:38

raymondfeng reviewed Jul 20, 2020

View reviewed changes

bajtos mentioned this pull request Jul 27, 2020

fix(authentication-jwt): move REST API Explorer to devDependencies #6009

Merged

7 tasks

This was referenced Aug 27, 2020

feat(rest): adds msgpack body parser #6155

Merged

feat: refactor filters into standalone package #6182

Merged

Put framework modules to peerDependencies in new extensions #6243

Merged

bajtos mentioned this pull request Sep 3, 2020

feat: move framework packages to peerDependencies #6264

Merged

3 tasks

raymondfeng mentioned this pull request Sep 4, 2020

feat(graphql): use peer dependencies for core and boot #6273

Merged

8 tasks

bajtos mentioned this pull request Sep 7, 2020

feat: move framework packages to peerDependencies #6288

Merged

3 tasks

bajtos closed this in #6288 Sep 8, 2020

achrinza mentioned this pull request Dec 22, 2020

Fixing dependencies to support yarn2 strict mode #6942

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor extensions to move framework modules to peerDependencies #5959

Refactor extensions to move framework modules to peerDependencies #5959

bajtos commented Jul 17, 2020

bajtos Jul 17, 2020

raymondfeng Jul 17, 2020

bajtos Jul 20, 2020

bajtos commented Jul 17, 2020

bajtos commented Jul 20, 2020

raymondfeng Jul 20, 2020

raymondfeng Jul 20, 2020

bajtos Jul 27, 2020

raymondfeng Jul 20, 2020

raymondfeng Jul 20, 2020

raymondfeng Jul 20, 2020

bajtos Jul 27, 2020

raymondfeng Jul 27, 2020

bajtos Jul 28, 2020

raymondfeng Jul 28, 2020

bajtos Aug 4, 2020 •

edited

Loading

raymondfeng Aug 13, 2020

bajtos commented Sep 1, 2020

Refactor extensions to move framework modules to peerDependencies #5959

Refactor extensions to move framework modules to peerDependencies #5959

Conversation

bajtos commented Jul 17, 2020

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Jul 17, 2020

bajtos commented Jul 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Aug 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Sep 1, 2020

bajtos Aug 4, 2020 •

edited

Loading