-
Notifications
You must be signed in to change notification settings - Fork 1.1k
DataSource Declarative CLI + Boot #1385
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
Changes from all commits
c737991
1d7b543
d317d32
3eba183
dc70c6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,3 +10,4 @@ packages/*/dist* | |
| examples/*/dist* | ||
| **/package | ||
| .sandbox | ||
| packages/cli/generators/datasource/connectors.json | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| --- | ||
| lang: en | ||
| title: 'DataSource generator' | ||
| keywords: LoopBack 4.0, LoopBack 4 | ||
| tags: | ||
| sidebar: lb4_sidebar | ||
| permalink: /doc/en/lb4/DataSource-generator.html | ||
| summary: | ||
| --- | ||
|
|
||
| {% include content/generator-create-app.html lang=page.lang %} | ||
|
|
||
| ### Synopsis | ||
|
|
||
| Adds new [DataSource](Datasources.md) class and config files to a LoopBack | ||
| application. | ||
|
|
||
| ```sh | ||
| lb4 datasource [options] [<name>] | ||
| ``` | ||
|
|
||
| ### Options | ||
|
|
||
| `--connector` : Name of datasource connector | ||
|
|
||
| This can be a connector supported by LoopBack / Community / Custom. | ||
|
|
||
| {% include_relative includes/CLI-std-options.md %} | ||
|
|
||
| ### Arguments | ||
|
|
||
| `<name>` - Required name of the datasource to create as an argiment to the | ||
| command. If provided, the tool will use that as the default when it prompts for | ||
| the name. | ||
|
|
||
| ### Interactive Prompts | ||
|
|
||
| The tool will prompt you for: | ||
|
|
||
| - **Name of the datasource.** _(dataSourceName)_ If the name had been supplied | ||
| from the command line, the prompt is skipped and the datasource is built with | ||
| the name from the command-line argument. | ||
| - **Name of connector.** If not supplied via command line, you will be presented | ||
| with a list of connector to select from (including an `other` option for | ||
| custom connector). | ||
| - **Connector configuration.** If the connector is not a custom connector, the | ||
| CLI will prompt for the connector configuration information. | ||
|
|
||
| ### Output | ||
|
|
||
| Once all the prompts have been answered, the CLI will do the following: | ||
|
|
||
| - Install `@loopback/repository` and the connector package (if it's not a custom | ||
| connector). | ||
| - Create a file with the connector configuration as follows: | ||
| `/datasources/${dataSource.dataSourceName}.datasource.json` | ||
| - Create a DataSource class which recieves the connector config using | ||
| [Dependency Injection](Dependency-injection.md) as follows: | ||
| `/datasources/${dataSource.dataSourceName}.datasource.ts` | ||
| - Update `index.ts` to export the newly created DataSource class. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| --- | ||
| lang: en | ||
| title: 'DataSources' | ||
| keywords: LoopBack 4.0, LoopBack 4 | ||
| tags: | ||
| sidebar: lb4_sidebar | ||
| permalink: /doc/en/lb4/DataSources.html | ||
| summary: | ||
| --- | ||
|
|
||
| ## Overview | ||
|
|
||
| A `DataSource` in LoopBack 4 is a named configuration for a Connector instance | ||
| that represents data in an external system. The Connector is used by | ||
| `legacy-juggler-bridge` to power LoopBack 4 Repositories for Data operations. | ||
|
|
||
| ### Creating a DataSource | ||
|
|
||
| It is recommended to use the [`lb4 datasource` command](DataSource-generator.md) | ||
| provided by the CLI to generate a DataSource. The CLI will prompt for all | ||
| necessary connector information and create the following files: | ||
|
|
||
| - `${dataSource.dataSourceName}.datasource.json` containing the connector | ||
| configuration | ||
| - `${dataSource.dataSourceName}.datasource.ts` containing a class extending | ||
| `juggler.DataSource`. This class can be used to override the default | ||
| DataSource behaviour programaticaly. Note: The connector configuration stored | ||
| in the `.json` file is injected into this class using | ||
| [Dependency Injection](Dependency-injection.md). | ||
|
|
||
| Both the above files are generated in `src/datasources/` directory by the CLI. | ||
| It will also update `src/datasources/index.ts` to export the new DataSource | ||
| class. | ||
|
|
||
| Example DataSource Class: | ||
|
|
||
| ```ts | ||
| import {inject} from '@loopback/core'; | ||
| import {juggler, DataSource} from '@loopback/repository'; | ||
|
|
||
| export class DbDataSource extends juggler.DataSource { | ||
| constructor(@inject('datasources.config.db') dsConfig: DataSource) { | ||
| super(dsConfig); | ||
| } | ||
| } | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| { | ||
| "name": "db", | ||
| "connector": "memory", | ||
| "localStorage": "", | ||
| "file": "./data/db.json" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,22 +3,17 @@ | |
| // This file is licensed under the MIT License. | ||
| // License text available at https://opensource.org/licenses/MIT | ||
|
|
||
| import * as path from 'path'; | ||
| // The juggler reference must exist for consuming code to correctly infer | ||
| // type info used in the "db" export (contained in juggler.DataSource). | ||
| // tslint:disable-next-line:no-unused-variable | ||
| import {juggler} from '@loopback/repository'; | ||
| import {inject} from '@loopback/core'; | ||
| import {juggler, DataSource} from '@loopback/repository'; | ||
| const config = require('./db.datasource.json'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I keep getting an error when I do this as follows: |
||
|
|
||
| const dsConfigPath = path.resolve( | ||
| __dirname, | ||
| '../../../config/datasources.json', | ||
| ); | ||
| const config = require(dsConfigPath); | ||
| export class DbDataSource extends juggler.DataSource { | ||
| static dataSourceName = 'db'; | ||
|
|
||
| // TODO(bajtos) Ideally, datasources should be created by @loopback/boot | ||
| // and registered with the app for dependency injection. | ||
| // However, we need to investigate how to access these datasources from | ||
| // integration tests where we don't have access to the full app object. | ||
| // For example, @loopback/boot can provide a helper function for | ||
| // performing a partial boot that creates datasources only. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please clarify how integration tests can access datasource configuration now? I guess the answer could be found somewhere in the code, but it will be easier for reviewers if you could summarize it in a comment.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One can access the config / DataSource by getting it from the context. I'm not sure of the question here ...
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In integration tests, there is no Application/Context object. We want to test the integration between a repository and the backing database, not the integration with the app/context. Also test-data-builders need to access a repository instance directly, see e.g. http://loopback.io/doc/en/lb4/Implementing-features.html#update-test-helpers-and-the-controller-use-real-model-and-repository async function givenEmptyDatabase() {
await new ProductRepository().deleteAll();
}
async function givenProduct(data: Partial<Product>) {
return await new ProductRepository().create(
Object.assign(
{
name: 'a-product-name',
slug: 'a-product-slug',
price: 1,
description: 'a-product-description',
available: true,
},
data,
),
);
}
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the scope of this pull request, it's enough to allow tests to instantiate const db = new DbDataSource();This can be achieved e.g. by adding a default value for I created a follow-up issue to improve the overall design to allow tests to modify the default datasource configuration, see #1396 |
||
| export const db = new juggler.DataSource(config); | ||
| constructor( | ||
| @inject('datasources.config.db', {optional: true}) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An idea to consider - out of scope of this pull request. @inject.optional('datasources.config.db')
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We allow |
||
| dsConfig: DataSource = config, | ||
| ) { | ||
| super(dsConfig); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| // Copyright IBM Corp. 2017,2018. All Rights Reserved. | ||
| // Node module: @loopback/example-todo | ||
| // This file is licensed under the MIT License. | ||
| // License text available at https://opensource.org/licenses/MIT | ||
|
|
||
| export * from './db.datasource'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,20 @@ describe('Application', () => { | |
| before(givenAnApplication); | ||
| before(async () => { | ||
| await app.boot(); | ||
|
|
||
| /** | ||
| * Override DataSource to not write to file for testing. Since we aren't | ||
| * persisting data to file and each injection normally instatiates a new | ||
| * instance, we must change the BindingScope to a singleton so only one | ||
| * instance is created and used for all injections (preserving access to | ||
| * the same memory space). | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Every datasource must be always bound in a singleton scope! A datasource holds a list of Model classes, a connection pool, etc. We cannot create a new datasource for each incoming HTTP request, juggler was not designed that way. |
||
| */ | ||
| app.bind('datasources.config.db').to({ | ||
| name: 'db', | ||
| connector: 'memory', | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. -1 for duplicating most of the configuration of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just binding
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess an alternative would be to do the merge here instead of the datasource -- I think I like this option better.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes please, that's what I had in mind 👍
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created a follow-up issue to improve the overall design to allow tests to modify the default datasource configuration, see #1396 For the scope of this pull request, I am proposing to go with a solution that's fastest to land. We can use the code snippet shown in https://github.com/strongloop/loopback-next/pull/1385/files#r192946885, or even leave
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. convention for naming or it's config? It's an optional binding a user can set to override the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope. It did in the earlier iteration but since we import the config this is the default key that gets set that a user can leverage in testing to override the default config if they wanted to -- it's injected with |
||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only a |
||
|
|
||
| // Start Application | ||
| await app.start(); | ||
| }); | ||
| before(givenARestServer); | ||
|
|
@@ -109,10 +123,6 @@ describe('Application', () => { | |
| rest: { | ||
| port: 0, | ||
| }, | ||
| datasource: { | ||
| name: 'db', | ||
| connector: 'memory', | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should commit the latest connectors.json. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pulled in as part of the
buildprocess and will be shipped with the appropriate version of the@loopback/clibecause we run build before a release.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the file is added to
.gitignore,npm publishwill skip it by default. Please check.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we have the
distfolders listed in.gitignoreand I've verified that they still get published tonpm.Reason being
npmsearches for.gitignorein each project root and sub-directories ... but for us the.gitignoreis at the top most level of the monorepo and not in any package hence no files are ignored.I also ran
npm packwithin theclipackage and can verify thatconnectors.jsonwas packed.