refactor: move non-core booters to their own packages #5618

raymondfeng · 2020-05-31T21:20:38Z

Inspired by the discussion at #5550, this PR cleans up @loopback/boot by moving non-core booters to their own packages.

The current @loopback/boot is becoming a kitchen sink for all booters and it drags in unnecessary dependencies to prevent itself from being a standalone package.

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

+1 to make @loopback/boot more lightweight.

-1 to make other existing packages more heavy weight.

So far, packages likes @loopback/repository and @loopback/rest-crud can be used independently from @loopback/boot, allowing applications to define their own bootstrapping strategy. I'd like to preserve that behavior.

If we want to extract the booter to their own packages, then please do that literally and create new packages to host the booters. For example: @loopback/repository-booters and @loopback/rest-crud-booters.

bajtos · 2020-06-02T13:05:36Z

@raymondfeng can you please post a short overview of booters to show which booters are going to stay in @loopbakc/boot and which are going to be moved elsewhere?

raymondfeng · 2020-06-02T21:43:46Z

So far, packages likes @loopback/repository and @loopback/rest-crud can be used independently from @loopback/boot, allowing applications to define their own bootstrapping strategy. I'd like to preserve that behavior.

Please note that the booters are contributions to the boot extension point. I don't think having their own booters in the same package will interfere with its functions.

If we want to extract the booter to their own packages, then please do that literally and create new packages to host the booters. For example: @loopback/repository-booters and @loopback/rest-crud-booters.

I wonder if it's over-engineering here. Consider the user experience - I would expect RepositoryComponent and CrudRestComponent to contribute their specific booters. Even if we create separate packages for booters, do you expect to have separate components and require all applications to import such components for app.boot to work?

The core booters left in @loopback/boot are:

base-artifact.booter.ts
application-metadata.booter.ts
interceptor.booter.ts
lifecyle-observer.booter.ts
component-application.booter.ts
controller.booter.ts
service.booter.ts

bajtos · 2020-07-20T14:29:04Z

So far, packages likes @loopback/repository and @loopback/rest-crud can be used independently from @loopback/boot, allowing applications to define their own bootstrapping strategy. I'd like to preserve that behavior.

Please note that the booters are contributions to the boot extension point. I don't think having their own booters in the same package will interfere with its functions.

If we want to extract the booter to their own packages, then please do that literally and create new packages to host the booters. For example: @loopback/repository-booters and @loopback/rest-crud-booters.

I wonder if it's over-engineering here. Consider the user experience - I would expect RepositoryComponent and CrudRestComponent to contribute their specific booters. Even if we create separate packages for booters, do you expect to have separate components and require all applications to import such components for app.boot to work?

I see your points. I agree there is a value in RepositoryComponent/RepositoryMixin providing a booter for repositories as part of the deal and that having a new package for each booter may be too much.

My main concern is about dependencies. As a user adding @loopback/repository to my @loopback/core based project using my custom bootstrapping conventions, I don't want to be forced to add a dependency on @loopback/boot that I am not going to use.

When I was designing Model API Builder/Booter, I solved this issue as follows:

The package @loopback/model-api-builder provides type definitions and implementation helpers for Model API Builder extension point.
Individual implementations (e.g. @loopback/rest-crud) depend on @loopback/model-api-builder directly, they treat model-api-builder as a building block.
@loopback/boot depends on @loopback/model-api-builder too, and exposes a Model API Booter that queries the context for all extensions.

Can we perhaps adopt a similar solution for @loopback/boot too?

Introduce a new package @loopback/booter to provide type definitions and implementation helpers for individual Booters. This package is meant to be used as a direct dependency.
Move RepositoryBooter to @loopback/repository. Instead of adding a peer dependency on @loopback/boot, we can add a regular dependency on the new package @loobpack/booter.
Unfortunately, there is no way how to express which @loopback/boot versions can be used together with @loopback/repository, because npm does not provide optional peer dependencies. (See also Rust Cargo's Features that allow consumers to pick features and thus sub-dependencies.) A possible solution is to check the compatibility at runtime: the base booter class can provide a version field, @loopback/boot can report an error when it encounters a booter with an incompatible version. I think we can leave this out for now, until there is a need to break the Booter contract in a backwards-incompatible way.

The core booters left in @loopback/boot are:

base-artifact.booter.ts

application-metadata.booter.ts

interceptor.booter.ts

lifecyle-observer.booter.ts

component-application.booter.ts

controller.booter.ts

service.booter.ts

This looks reasonable 👍

I have a question regarding the datasource booter. Both repositories (@loopback/repository) and service proxies (@loopback/service-proxy) need datasources. IIUC, your current proposal is to put the datasource booter to @loopback/repository only, therefore applications using @loopback/service-proxy without @loopback/repository will NOT be able to bootstrap datasources. I find that as too limiting and probably difficult to discover & understand for our users too. Can we find a better solution please?

raymondfeng · 2020-07-20T16:51:41Z

My main concern is about dependencies. As a user adding @loopback/repository to my @loopback/core based project using my custom bootstrapping conventions, I don't want to be forced to add a dependency on @loopback/boot that I am not going to use.

I think this is a good use case of peerDependencies. If the @loopback/boot is declared as a peer dependency (ideally optional peer dependency) for @loopback/repository, we'll have the following benefits:

If another package uses @loopback/repository, it does not transitively drags in @loopback/boot
When @loopback/boot is present for the application, booters from @loopback/repository will be discovered and executed for app.boot.

raymondfeng · 2020-07-20T16:52:41Z

I have a question regarding the datasource booter. Both repositories (@loopback/repository) and service proxies (@loopback/service-proxy) need datasources. IIUC, your current proposal is to put the datasource booter to @loopback/repository only, therefore applications using @loopback/service-proxy without @loopback/repository will NOT be able to bootstrap datasources. I find that as too limiting and probably difficult to discover & understand for our users too. Can we find a better solution please?

I agree. There are quite a bit common code that should be ideally extracted from @loopback/repository.

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

Introduce a new package @loopback/booter to provide type definitions and implementation helpers for individual Booters. This package is meant to be used as a direct dependency.

I initially had @loopback/bootstrap for this purpose and it turned out making things unnecessarily complicated, especially for backward compatibility.

If @loopback/boot becomes a peer dependency for those packages that contribute booters, having interfaces and bootstrapper (extension point) is not bad and heavy.

There is an interesting connection between boot and core. Do we want to have core-booters in its own package?

raymondfeng · 2020-09-22T16:37:09Z

@bajtos I have rebased this PR against the latest master. I also adopted the peerDependencies for @loopback/boot, which should remove the key concerns you had. PTAL.

raymondfeng · 2020-10-13T19:37:04Z

@bajtos Thank you for the input. I have refactored common types/interfaces for booters into @loopback/booter so that other packages can use it as the peer dependency to implement custom booters.

PTAL.

jannyHou

Following discussions, the latest refactor looks reasonable to me 👍 Had a chat with Raymond, the next step could be extracting the datasource booter to a standalone package.

jannyHou · 2020-10-19T21:11:49Z

packages/boot/src/__tests__/acceptance/controller.booter.acceptance.ts

@@ -44,8 +33,4 @@ describe('controller booter acceptance tests', () => {
      rest: givenHttpServerConfig(),
    });
  }
-
-  async function stopApp() {


no need to app.stop()?

jannyHou · 2020-10-19T21:23:32Z

packages/repository/src/mixins/repository.mixin.ts

+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    constructor(...args: any[]) {
+      super(...args);
+      const ctx = (this as unknown) as Context;


is it safe to assume the RepositoryMixin always applies to a Context?

The review is no longer relevant.

bajtos · 2020-10-20T15:33:51Z

Thank you @raymondfeng for addressing my comments. I am afraid I won't be able to continue the review in a timely manner and at the same time, I don't want to block progress. I am ok to leave the final review and approval to @jannyHou.

One use case to consider for testing - I think it's showing that the current design of dependencies & peer dependencies is not going to work as desired.

The idea is to create a small application that depends only on @loopback/core and @loopback/repository (see #5618 (comment) for longer explanation). Because the new version of @loopback/repository is not published to npm yet, we need to use a local copy. However, we also must not use lerna to setup dependencies, because lerna is not reporting missing peer dependencies as npm does.

Pack the repository package:
```
$ (cd packages/repository && npm pack)
```

Create a new sandbox directory sandbox/test and put the following package.json file there:

{
  "name": "test",
  "dependencies": {
    "@loopback/core": "^2.11.0",
    "@loopback/repository": "file:../../packages/repository/loopback-repository-3.1.0.tgz"
  }
}

Install dependencies in our test project:
```
$ (cd sandbox/test && npm i)
```

Observe a warning about a missing peer dependency:

npm WARN @loopback/[email protected] requires a peer of @loopback/booter@^1.0.0 but none is installed. You must install peer dependencies yourself.

Signed-off-by: Raymond Feng <[email protected]> BREAKING CHANGE: Model/DataSource/Repository/ModelApi booters are moved to `@loopback/repository` and `@loopback/model-api-builder` packages. There are no breaking changes for other APIs. Existing applications should continue to work unless they use such booters explicitly.

Signed-off-by: Raymond Feng <[email protected]>

stale · 2021-07-14T05:44:17Z

This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity.

stale · 2021-07-28T06:10:50Z

This pull request has been closed due to continued inactivity. If you are interested in finishing the proposed changes, then feel free to re-open this pull request or open a new one.

raymondfeng requested review from bajtos and jannyHou as code owners May 31, 2020 21:20

raymondfeng force-pushed the refactor-boot branch 2 times, most recently from bc2d8c0 to 997dd0d Compare May 31, 2020 23:38

raymondfeng requested review from agnes512 and hacksparrow as code owners May 31, 2020 23:38

raymondfeng changed the title ~~[RFC] refactor @loopback/boot into @loopback/bootstrap and @loopback/boot~~ [RFC] move non-core booters to their own packages May 31, 2020

raymondfeng force-pushed the refactor-boot branch from 997dd0d to a30c458 Compare June 1, 2020 16:45

raymondfeng self-assigned this Jun 1, 2020

raymondfeng added Boot breaking-change labels Jun 1, 2020

bajtos previously requested changes Jun 2, 2020

View reviewed changes

bajtos added the refactor label Jun 2, 2020

raymondfeng force-pushed the refactor-boot branch from a30c458 to 319f809 Compare June 20, 2020 02:02

raymondfeng force-pushed the refactor-boot branch from 319f809 to a9ad6e3 Compare July 6, 2020 21:59

raymondfeng force-pushed the refactor-boot branch from a9ad6e3 to 081b9af Compare July 14, 2020 16:02

raymondfeng mentioned this pull request Jul 17, 2020

Refactor extensions to move framework modules to peerDependencies #5959

Closed

5 tasks

raymondfeng force-pushed the refactor-boot branch from 081b9af to 8873690 Compare July 22, 2020 18:18

raymondfeng force-pushed the refactor-boot branch from 8873690 to 13c4343 Compare August 19, 2020 16:50

raymondfeng requested a review from nabdelgadir as a code owner August 19, 2020 16:50

raymondfeng force-pushed the refactor-boot branch from 13c4343 to 7ff6810 Compare August 27, 2020 22:08

raymondfeng force-pushed the refactor-boot branch from 7ff6810 to 4bc457f Compare September 22, 2020 15:39

raymondfeng changed the title ~~[RFC] move non-core booters to their own packages~~ refactor: move non-core booters to their own packages Sep 22, 2020

raymondfeng force-pushed the refactor-boot branch 4 times, most recently from 7103c15 to fc5beeb Compare October 13, 2020 18:15

raymondfeng requested a review from emonddr as a code owner October 13, 2020 18:15

raymondfeng force-pushed the refactor-boot branch from fc5beeb to f32df14 Compare October 13, 2020 19:14

dhmlau added this to the Oct 2020 milestone Oct 14, 2020

raymondfeng requested a review from bajtos October 15, 2020 20:23

dhmlau assigned jannyHou Oct 19, 2020

jannyHou reviewed Oct 19, 2020

View reviewed changes

raymondfeng added 8 commits October 29, 2020 08:48

feat(repository): add booters for datasources, repositories, and models

cd7abe9

Signed-off-by: Raymond Feng <[email protected]>

feat(model-api-builder): add model api booter

016e39d

Signed-off-by: Raymond Feng <[email protected]>

feat(rest-crud): add model api booter in the CrudRestComponent

c573883

Signed-off-by: Raymond Feng <[email protected]>

feat(booter): add @loopback/booter for common boot interfaces

065f8ad

Signed-off-by: Raymond Feng <[email protected]>

feat(boot): use @loopback/booter for booter related dependencies

3d0402d

Signed-off-by: Raymond Feng <[email protected]>

feat: use @loopback/booter as the peer dependency

27d299b

Signed-off-by: Raymond Feng <[email protected]>

feat(cli): add @loopback/booter to project dependencies

0cf4cff

Signed-off-by: Raymond Feng <[email protected]>

raymondfeng force-pushed the refactor-boot branch from 7100ea4 to 0cf4cff Compare October 29, 2020 15:48

raymondfeng requested a review from dougal83 as a code owner October 29, 2020 15:48

dhmlau modified the milestones: Oct 2020, Nov 2020 Nov 2, 2020

dhmlau removed this from the Nov 2020 milestone Dec 6, 2020

stale bot added the stale label Jul 14, 2021

stale bot closed this Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move non-core booters to their own packages #5618

refactor: move non-core booters to their own packages #5618

raymondfeng commented May 31, 2020 •

edited

Loading

bajtos left a comment

bajtos commented Jun 2, 2020

raymondfeng commented Jun 2, 2020

bajtos commented Jul 20, 2020

raymondfeng commented Jul 20, 2020

raymondfeng commented Jul 20, 2020

raymondfeng commented Jul 20, 2020 •

edited

Loading

raymondfeng commented Sep 22, 2020

raymondfeng commented Oct 13, 2020

jannyHou left a comment

jannyHou Oct 19, 2020

jannyHou Oct 19, 2020

bajtos commented Oct 20, 2020

stale bot commented Jul 14, 2021

stale bot commented Jul 28, 2021

refactor: move non-core booters to their own packages #5618

refactor: move non-core booters to their own packages #5618

Conversation

raymondfeng commented May 31, 2020 • edited Loading

Checklist

bajtos left a comment

Choose a reason for hiding this comment

bajtos commented Jun 2, 2020

raymondfeng commented Jun 2, 2020

bajtos commented Jul 20, 2020

raymondfeng commented Jul 20, 2020

raymondfeng commented Jul 20, 2020

raymondfeng commented Jul 20, 2020 • edited Loading

raymondfeng commented Sep 22, 2020

raymondfeng commented Oct 13, 2020

jannyHou left a comment

Choose a reason for hiding this comment

jannyHou Oct 19, 2020

Choose a reason for hiding this comment

jannyHou Oct 19, 2020

Choose a reason for hiding this comment

bajtos commented Oct 20, 2020

stale bot commented Jul 14, 2021

stale bot commented Jul 28, 2021

raymondfeng commented May 31, 2020 •

edited

Loading

raymondfeng commented Jul 20, 2020 •

edited

Loading