DataSource Declarative CLI + Boot #1385

virkt25 · 2018-05-31T00:08:08Z

NOTE: This is based on top of the indexer PR -- which has feedback that I will be working to address so it can land before this PR.

fixes #1225

PR is broken by commits for easier review

feat(cli): add lb4 datasource command …
fix(repository): accept class instead of instance for app.datasource …
feat(boot): add datasource and datasource config booters …
refactor(example-todo): use datasource cli generated files and dataso… …
docs(docs): update docs with datasource cli and booter content …

PENDING:

Booter Tests
Example Todo Tests
Example Todo Docs
Other Docs
PR fix(testlab): decache files in TestSandbox when resetting it #1391 needs to land first
PR feat(cli): auto-generate index.ts for exports #1363 needs to land first

Checklist

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

shimks

Great work! I'll leave the high-level reviews to the senior developers.
It might be better to leave out the make-index stuff outside of this PR

shimks · 2018-06-01T14:59:54Z

packages/cli/generators/datasource/connectors.json

@@ -0,0 +1,750 @@
+{


Is there a way to use the json file in loopback-workspace? Having a copy like this may be hard to maintain

+1 for avoiding the need to maintain two identical lists.

I don't like the idea of depending on loopback-workspace, as that would add too many dependencies including LoopBack 3.x

What we can do instead:

Move connectors.json from loopback-worspace into a new independent npm package. We can start a new GitHub project or keep the new package in loopback-next monorepo.

Keep connectors in loopback-workspace, add a build step to @loopback/cli to download the current/latest version of connectors from GitHub (URL: https://raw.githubusercontent.com/strongloop/loopback-workspace/master/available-connectors.json)

Thoughts?

cc @raymondfeng

shimks · 2018-06-01T15:16:08Z

packages/cli/test/integration/generators/datasource.integration.js

+describe('lb4 datasource', () => {
+  beforeEach('reset sandbox', () => sandbox.reset());
+
+  it('does not run without package.json', () => {


We might need to look into separating out the common tests across artifact generators like this one. Outside of scope in context of this PR though

bajtos · 2018-06-04T08:34:02Z

packages/cli/generators/datasource/templates/datasource.ts.ejs

+import {juggler, DataSource} from '@loopback/repository';
+
+export class <%= className %>DataSource extends juggler.DataSource {
+  constructor(@inject('datasources.config.<%= name %>') dsConfig: DataSource) {


dsConfig: DataSource looks suspicious to me.

export interface DataSource { name: string; // Name of the data source connector?: Connector; // The underlying connector instance settings: AnyObject; // Settings // tslint:disable-next-line:no-any [property: string]: any; // Other properties that vary by connectors }

Configuration wise, we allow developers to specify the connector as a string, e.g. 'mysql' instead of require('loopback-connector-mysql').

DataSource configuration does not contain any settings object, these settings (e.g. url, database, etc.) are top-level properties of the config object.

I think you should use juggler.Options type instead, see juggler.DataSource constructors and [definition of Options](https://github.com/strongloop/loopback-datasource-juggler/blob/77f11cda3b6e9e39c1cc039808180129a23d67ff/types/common.d.ts#L13-L16.

To make these datasource classes easier to use in tests, I think dsConfig should have a default value.

import * as CONFIG from './<%= name %>.json'; export {CONFIG}; export class <%= className %>DataSource extends juggler.DataSource { constructor(@inject('datasources.config.<%= name %>') dsConfig: juggler.Options = CONFIG) { super(dsConfig); } }

Thoughts?

Hmm ... I had used DI for the config based on your comment in the boot spike. That said looking at the case of overriding the dsConfig by binding a partial value (such as setting file to undefined in examples/todo) I think it might be better to merge the default config from the file with the config that is bound to Context ... only question is that is that what a user would expect?

import * as CONFIG from './<%= name %>.json'; export class <%= className %>DataSource extends juggler.DataSource { constructor(@inject('datasources.config.<%= name %>') dsConfig: juggler.Options = {}) { super(Object.assign(CONFIG, dsConfig)); } }

Thoughts? ... I'd like agreement before I go about making changes

^ With that my only concern is what a developer expects ... do they expect config that lives in context to be used or is a merge what they expect?

With regards to the first comment, I wasn't too sure of the purpose of the settings object since I did realize that the settings are actually a top level object. I can change it to juggler.Options --

What is the purpose of the DataSource interface as it's defined here? -- Is it just a remnant of something before the refactor / move to juggler.DataSource?

What is the purpose of the DataSource interface as it's defined here? -- Is it just a remnant of something before the refactor / move to juggler.DataSource?

No idea. @raymondfeng could you please clarify?

I had used DI for the config based on your comment in the boot spike.

Sure, I am fine with using DI, I think the Todo example app is already showing why we need that.

I think it might be better to merge the default config from the file with the config that is bound to Context .

That's a bad idea based on my past experience with loopback-boot. The trouble with merging is that users wishing to obtain a specific datasource configuration must unset all unwanted properties provided in the "default" config, which is pretty much impossible.

For example, consider the case where you want to configure a MySQL datasource to use a specific user+password+database combination while ensuring that no other datasource options are set. Now imagine somebody modifies the JSON configuration and adds let's say charset property. Now we need to update the code wanting only user+password+database code to exclude charset from the original config.

Thoughts?

I would like to use import for the json directly within the generated TS file (supported by 2.9.x - https://blogs.msdn.microsoft.com/typescript/2018/05/31/announcing-typescript-2-9/#json-imports)

my-db.datasource.ts

import config from './my-db.json'; // Unfortunately it cannot be `my-db.datasource.json`. export class MyDBDataSource extends juggler.DataSource { constructor() { super(config); } }

bajtos · 2018-06-04T08:39:06Z

packages/cli/generators/datasource/connectors.json

@@ -0,0 +1,750 @@
+{


+1 for avoiding the need to maintain two identical lists.

I don't like the idea of depending on loopback-workspace, as that would add too many dependencies including LoopBack 3.x

What we can do instead:

Move connectors.json from loopback-worspace into a new independent npm package. We can start a new GitHub project or keep the new package in loopback-next monorepo.

Keep connectors in loopback-workspace, add a build step to @loopback/cli to download the current/latest version of connectors from GitHub (URL: https://raw.githubusercontent.com/strongloop/loopback-workspace/master/available-connectors.json)

Thoughts?

cc @raymondfeng

bajtos · 2018-06-04T08:44:09Z

examples/todo/test/acceptance/application.acceptance.ts

+     */
+    app.bind('datasources.config.db').to({
+      name: 'db',
+      connector: 'memory',


-1 for duplicating most of the configuration of db datasource. Could you please try to find a way how to rewrite this code so that it changes only the relevant flags (e.g. set file to undefined - I don't remember exact memory config options)?

Just binding {file: undefined}won't work as a DataSource needs the name and connector. See my comment above with regards to merging the bound value with the default config.

I guess an alternative would be to do the merge here instead of the datasource -- I think I like this option better.

const dsKey = 'datasources.config.db'; const config = await app.get(dsKey); app.bind(dsKet).to(Object.assign(config, {file: undefined}));

I guess an alternative would be to do the merge here instead of the datasource -- I think I like this option better.

Yes please, that's what I had in mind 👍

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 {file: undefined} change out of scope and simply use the same datasource config as the application uses at runtime.

Does datasources.config.db follow any convention?

convention for naming or it's config? It's an optional binding a user can set to override the DataSource config (default coming from the generated JSON file). This is more for testing ... with #1396 looking to improve this further.

Does @loopback/boot bind datasource config to datasources.config.<dsName>?

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 {optional: true}

bajtos · 2018-06-04T08:45:27Z

examples/todo/test/acceptance/application.acceptance.ts

+     * 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).


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.

bajtos · 2018-06-04T08:46:43Z

examples/todo/src/datasources/db.datasource.ts

-// 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.


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.

One can access the config / DataSource by getting it from the context. I'm not sure of the question here ...

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, ), ); }

For the scope of this pull request, it's enough to allow tests to instantiate DbDataSource without the need to create a custom datasource config:

const db = new DbDataSource();

This can be achieved e.g. by adding a default value for dsConfig argument, as discussed and shown in #1385 (comment).

I created a follow-up issue to improve the overall design to allow tests to modify the default datasource configuration, see #1396

bajtos · 2018-06-04T08:47:55Z

examples/todo/test/acceptance/application.acceptance.ts

+      name: 'db',
+      connector: 'memory',
+    });
+    app.find('datasources.db')[0].inScope(BindingScope.SINGLETON);


See my earlier comment - BindingScope.SINGLETON must be applied to all datasources by @loopback/boot.

bajtos · 2018-06-04T08:55:15Z

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

@@ -97,9 +97,11 @@ export function RepositoryMixin<T extends Class<any>>(superClass: T) {
     * }
     * ```
     */
-    dataSource(dataSource: juggler.DataSource, name?: string) {
+    dataSource(dataSource: Class<juggler.DataSource>, name?: string) {


I feel LB4 apps should not be required to always define a new DataSource class. Can we support dual usage allowing both a DataSource instance and a DataSource class?

dataSource(dataSource: juggler.DataSource | Class<juggler.DataSource>, name? string): void { if (typeof juggler.DataSource === 'function') { // handle the Class case } else { // handle the instance case } }

Thoughts?

Is there a use case you have in mind where someone will prefer to create an instance instead of a DataSource class?

import {Application} from '@loopback/core'; import {juggler} from '@loopback/repository'; const app = new Application(); app.dataSource(new juggler.DataSource('db', {connector: 'memory'})); // etc.

While this is not a typical usage in LB4 apps scaffolded by our CLI tooling, it makes it much much simpler to write short scripts reproducing certain behavior (e.g. a bug).

bajtos · 2018-06-04T08:59:00Z

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

@@ -97,9 +97,11 @@ export function RepositoryMixin<T extends Class<any>>(superClass: T) {
     * }
     * ```
     */
-    dataSource(dataSource: juggler.DataSource, name?: string) {
+    dataSource(dataSource: Class<juggler.DataSource>, name?: string) {
      const dataSourceKey = `datasources.${name || dataSource.name}`;


Using dataSource.name as the default name value changes the behavior quite a lot.

Before:

app.dataSource(new DataSource({name: 'db', connector: 'memory'})); // -> name is "db"

After your change:

app.bind('datasources.config.db').to(({name: 'db', connector: 'memory'}); app.dataSource(DbDataSource); // -> name is DbDataSource

IMO, the name argument must be required when registering a datasource via a class.

The boot module tries to mitigate this by passing in a name to app.dataSource where it removes the suffix DataSource -- if present.

IMO, this API needs to support more than just our boot module. We want to encourage people to build their own boot-strapper if they don't like our convention or have a specific use case were our conventions don't work well.

One use case is to write a short script that reproduces certain behavior and calls all LB APIs directly, without going through the boot module. Our API should guide the user to do the right thing, i.e. ask them to provide a datasource name if they are passing a class constructor.

bajtos · 2018-06-04T08:59:27Z

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

-      this.bind(dataSourceKey).to(dataSource);
+      this.bind(dataSourceKey)
+        .toClass(dataSource)
+        .tag('datasource');


As mentioned in another comment, we must change the scope of all datasource binding to a SINGLETON.

virkt25 · 2018-06-05T05:12:09Z

packages/boot/src/booters/datasource.booter.ts

+        );
+      } else {
+        this.classes.forEach(cls => {
+          const name = kebabCase(cls.name.replace('DataSource', ''));


@bajtos This is where the name for binding the DataSource class gets changed from DbDataSource to db.

Now that I look at it, maybe this would be better in the .dataSource function so anyone using it directly also benefits from this and not just boot users. Thoughts?

IMO, this adds too much coupling between our core/repository APIs, the way how boot operates and the naming conventions we use in files generated by our tooling.

Ideally, the core & repository APIs should be agnostic of any class-naming conventions.

Also, what if we have DbDataSource class with JSON config file that says name: "mysql-db" (imagine we started with the same name but then a change was made in one of these two places only)? Which name will take precedence? How are our users going to learn about these precedence rules? Isn't this making the learning curve more steeper than needed?

raymondfeng · 2018-06-05T21:17:09Z

packages/cli/generators/datasource/index.js

+   * Ask for DataSource Name -- Must be unique
+   */
+  promptArtifactName() {
+    return super.promptArtifactName();


We should prompt for the name of a datasource, not the class name.

virkt25 · 2018-06-06T02:51:21Z

Talked with @raymondfeng and agreed with on the following:

Import JSON from local file while have DI as an optional way of overriding the default config.
CLI will prompt for DataSource name - Which will be set as a static property on the DataSource Class -- and used by app.dataSource for binding so we aren't "magically" guessing the name. The static property can be removed once we have @injectable providing binding metadata (non-DP3)
app.dataSource will support binding a juggler.DataSource class / instance / config (create instance in memory)

Outstanding Questions / Todos / Concerns

Maintaining 2 lists for the CLI DataSources? -- PROPOSAL: Update this in @loopback/cli by getting the file from loopback-workspace master -- I am ok with this.
Create a follow up task on better re-factoring of CLI tests common to all generators -- out of scope of this PR

bajtos · 2018-06-06T06:44:58Z

Import JSON from local file while have DI as an optional way of overriding the default config.

+1

CLI will prompt for DataSource name - Which will be set as a static property on the DataSource Class -- and used by app.dataSource for binding so we aren't "magically" guessing the name. The static property can be removed once we have @Injectable providing binding metadata (non-DP3)

+1

Please note that name is a reserved property of functions/class constructors and you need to use a different static property. In LB 3.x, we use modelName for model names. Perhaps you can use dataSourceName here?

app.dataSource will support binding a juggler.DataSource class / instance / config (create instance in memory)

+1 to support more flavors.

I have a concern though - can we reliably distinguish between a DataSource instance and a DataSource config object? Both values are an object. (DataSource class is a function, that's easy to tell apart.)

I have posted two comments regarding application integration/acceptance tests, I hope they will help us to keep the scope of this PR small(er) and get it landed sooner:

See also the follow-up issue I created: #1396 How to modify datasource configuration in integration/acceptance tests

bajtos

The new code looks mostly good, I have few minor comments to address.

bajtos · 2018-06-12T12:17:00Z

docs/site/DataSource-generator.md

+- Install `@loopback/repository` and the connector package (if it's not a custom
+connector).
+- Create a file with the connector configuration as follows:
+  `/datasources/${connector.name}.datasource.json`


I think the file name should be based on datasource name provided by the user (e.g. db), not the connector name (e.g. memory), because a single connector can be used for multiple datasources.

The same comment applies to the next line below.

bajtos · 2018-06-12T12:17:25Z

docs/site/DataSources.md

+provided by the CLI to generate a DataSource. The CLI will prompt for all necessary connector information and create the following files:
+
+- `${connector.name}.datasource.json` containing the connector configuration
+- `${connector.name}.datasource.ts` containing a class extending `juggler.DataSource`. This class can be used to override the default DataSource


Ditto, I think we should use dataSourceName instead of connector.name.

bajtos · 2018-06-12T12:18:08Z

docs/site/todo-tutorial-datasource.md

+? window.localStorage key to use for persistence (browser only):
+? Full path to file for persistence (server only): ./data/db.json
+
+  create src/datasources/ds.datasource.json


The prompt shows Datasource name: db, but the file name is ds.datasource.json. I think it should be db.datasource.json?

bajtos · 2018-06-12T12:19:34Z

examples/todo/src/datasources/db.datasource.ts

-import {juggler} from '@loopback/repository';
+import {inject} from '@loopback/core';
+import {juggler, DataSource} from '@loopback/repository';
+const config = require('./db.datasource.json');


Can we use import * as config from './db.datasource.json' to get more type information?

I keep getting an error when I do this as follows: Type 'typeof FILENAME' is not assignable to type 'DataSource' --- and the new import from JSON in TS is in 2.9 but we had to revert back to 2.8. This will have to be addressed in a follow up PR once we can use TS 2.9

bajtos · 2018-06-12T12:20:17Z

examples/todo/src/datasources/db.datasource.ts

-// performing a partial boot that creates datasources only.
-export const db = new juggler.DataSource(config);
+  constructor(
+    @inject('datasources.config.db', {optional: true})


An idea to consider - out of scope of this pull request.

@inject.optional('datasources.config.db')

@inject.optional doesn't exist yet in our code but I do like the idea.

We allow @inject('datasources.config.db', {optional: true});

bajtos · 2018-06-12T12:24:26Z

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

-    dataSource(dataSource: juggler.DataSource, name?: string) {
-      const dataSourceKey = `datasources.${name || dataSource.name}`;
-      this.bind(dataSourceKey).to(dataSource);
+    dataSource(dataSource: Class<juggler.DataSource>, name?: string) {


I think we agreed that app.dataSource should allow both Class<juggler.DataSource> and juggler.DataSource?

most comments have been addressed

raymondfeng · 2018-06-13T15:13:23Z

packages/cli/bin/download-connector-list.js

+async function download() {
+  var file = fs.createWriteStream(dest);
+  var request = https
+    .get(URL, function(response) {


Hmm, why don't we keep a local copy with @loopback/cli?

I realize that it's used as part of the build.

Yep. Build pulls in the latest from loopback-workspace as we didn't want to maintain two lists of connectors & didn't want to make loopback-workspace a dependency.

raymondfeng · 2018-06-13T15:15:19Z

packages/cli/bin/download-connector-list.js

+    });
+}
+
+async function transformConnectorJSON() {


Can we come up a json format to have more metadata about the prompts?

Not sure what you mean -- using the same format as loopback-workspace but transformed to be simpler for consumption by CLI prompts -- in LB3 the CLI does the transformation when the command is called, I am preferring to do it whenever we get the file.

raymondfeng

I left a few comments which can be addressed later.

b-admike · 2018-06-13T15:42:03Z

packages/cli/generators/datasource/index.js

+      debug(`npmModule - ${pkgs[0]}`);
+    }
+
+    pkgs.push('@loopback/repository');


would it make sense to install @loopback/core here as well since we import inject from it in the template datasource ejs file?

This command is meant to be run inside a loopback 4 project which if generated using CLI should already have @loopback/core installed but we don't install @loopback/repository.

b-admike · 2018-06-13T15:46:22Z

packages/cli/lib/utils.js

+ * Get a validate function for object/array type
+ * @param {String} type 'object' OR 'array'
+ */
+exports.validateStringObject = function(type) {


function(type: string) here?

CLI is a JS project not a TS project so no types here. -- In the future we should definitely consider switching to TS.

b-admike · 2018-06-13T15:48:04Z

packages/cli/lib/utils.js

+      return true;
+    }
+
+    const err = `The value must be a stringified ${type}`;


is this error message applicable to all types of errors we get below?

Yep. This function comes from LB3 DataSource CLI command and it's the same error.

Here's the original reference code: https://github.com/strongloop/generator-loopback/blob/master/lib/helpers.js#L210-L228

shimks

I know this PR's been ongoing forever and you want to merge it asap, so sorry to 🌧 on your 🎪 xd.

Aside from my comments below, we also need to update boot/docs.json for the new files.

Also a question, what are the differences between datasources being bound to dataSource.config.NAME key vs dataSource.NAME key?

shimks · 2018-06-13T14:57:33Z

docs/site/DataSource-generator.md

+
+### Synopsis
+
+Adds new [DataSource] class and config files to a LoopBack application.


Missing link here

shimks · 2018-06-13T15:01:39Z

docs/site/DataSource-generator.md

+- 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.html) as follows:


Dependency-injection.md
I just realized this is probably really inconsistent among our docs and probably needs to be fixed at one point (outside of scope).

shimks · 2018-06-13T15:02:50Z

docs/site/DataSources.md

+### Creating a DataSource
+
+It is recommended to use the `lb4 datasource`
+[command](DataSource-generator.html) provided by the CLI to generate a


nitpick: wrap the link around lb4 source as well

shimks · 2018-06-13T15:03:12Z

docs/site/DataSources.md

+  `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-inecjtion.html).


wrong spelling for the link and should me md

shimks · 2018-06-13T15:21:11Z

packages/cli/bin/download-connector-list.js

+      response.pipe(file);
+      file.on('finish', function() {
+        file.close();
+        transformConnectorJSON();


shimks · 2018-06-13T15:31:54Z

packages/cli/generators/datasource/index.js

+   * Ask for DataSource Name -- Must be unique
+   */
+  promptArtifactName() {
+    debug('Prompting for artifact name');


Any reason to not just super.promptArtifactName() here?

Yep. super.promptArtifactName() prompts for utils.toClassName(this.artifactInfo.type) + 'class name:' but we need to just prompt for DataSource name and not class name for this generator.

shimks · 2018-06-13T15:49:22Z

.gitignore

@@ -10,3 +10,4 @@ packages/*/dist*
 examples/*/dist*
 **/package
 .sandbox
+packages/cli/generators/datasource/connectors.json


I wonder if we should commit the latest connectors.json. Thoughts?

It's pulled in as part of the build process and will be shipped with the appropriate version of the @loopback/cli because we run build before a release.

If the file is added to .gitignore, npm publish will skip it by default. Please check.

But we have the dist folders listed in .gitignore and I've verified that they still get published to npm.

Reason being npm searches for .gitignore in each project root and sub-directories ... but for us the .gitignore is at the top most level of the monorepo and not in any package hence no files are ignored.

I also ran npm pack within the cli package and can verify that connectors.json was packed.

shimks · 2018-06-13T16:11:06Z

packages/cli/test/integration/generators/datasource.integration.js

+const testlab = require('@loopback/testlab');
+
+const expect = testlab.expect;
+const TestSandbox = testlab.TestSandbox;


As a side, what's preventing us from using the import _ from _ pattern here?

This is a JS project, not a TS project which is where the import _ from _. Support for this will come to JS eventually -- current status is Unstable as documented here: https://nodejs.org/api/esm.html

shimks · 2018-06-13T16:21:21Z

packages/cli/lib/utils.js

+ * Get a validate function for object/array type
+ * @param {String} type 'object' OR 'array'
+ */
+exports.validateStringObject = function(type) {


This utility function needs a test of its own

shimks · 2018-06-13T16:26:08Z

packages/boot/package.json

    "debug": "^3.1.0",
-    "glob": "^7.1.2"
+    "glob": "^7.1.2",
+    "lodash": "^4.17.10"


I don't see any changes with references to lodash. Probably left in as a mistake?

Good catch! I was using it for determining the binding name but we since switched to the static dataSourceName property for that ... and as such no longer need lodash.

b-admike · 2018-06-13T16:28:36Z

examples/todo/test/acceptance/application.acceptance.ts

+    app.bind('datasources.config.db').to({
+      name: 'db',
+      connector: 'memory',
+    });


.inScope(BindingScope.SINGLETON) here? (IIUC from the comment above)

Only a DataSource instance needs to be bound as a singleton. The config -- which is just the declarative JSON config can be bound as normal to the context.

shimks · 2018-06-13T16:31:18Z

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

@@ -155,7 +174,7 @@ export interface AppWithRepository extends Application {
  repository(repo: Class<any>): void;
  // tslint:disable-next-line:no-any
  getRepository<R extends Repository<any>>(repo: Class<R>): Promise<R>;
-  dataSource(dataSource: juggler.DataSource, name?: string): void;
+  dataSource(dataSource: Class<juggler.DataSource>, name?: string): void;


Class<juggler.DataSource> | juggler.DataSource

shimks · 2018-06-13T16:31:37Z

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

@@ -236,7 +255,7 @@ export class RepositoryMixinDoc {
   * }
   * ```
   */
-  dataSource(dataSource: juggler.DataSource, name?: string) {}
+  dataSource(dataSource: Class<juggler.DataSource>, name?: string) {}


Class<juggler.DataSource> | juggler.DataSource

Signed-off-by: Taranveer Virk <[email protected]>

…urce booters

bajtos · 2018-06-14T07:59:00Z

packages/cli/bin/download-connector-list.js

+  var request = https
+    .get(URL, function(response) {
+      response.pipe(file);
+      file.on('finish', async function() {


@virkt25 this is not ideal.

If the async function fails, it returns a promise that will trigger unhandled rejection warning. I think you need to wrap the async function with a regular one that installs .catch handler.

I am not sure what happens if the file cannot be written - will response.pipe(file) forward the error from file to response stream? I don't think so. Have you considered using await pEvent(file, 'finish') instead?

Alternatively, I think it may be easier to give up on async/await in this script and switch to callbacks . You are already using callbacks in several places, I think the code will become more consistent if callbacks are used everywhere.

Also, IIUC this code, the async function download will immediately return a fulfilled promise - it's not waiting for the request to finish. If it's intentional, then please remove the async keyword.

bajtos · 2018-06-14T08:02:56Z

packages/cli/bin/download-connector-list.js

+ * - Transforms description to message so it can be used by CLI directly
+ */
+async function transformConnectorJSON() {
+  let data = await readFileAsync(DEST, 'utf-8');


You are saving the downloaded JSON content to a file first, then read the file and overwrite its content in the second step. Have you considered passing the JSON content between download and transformConnectorJSON via in-memory value?

My recommendation is to use request-promise-native, it will take care of stream processing, error handling and flow control for you, and also add support for nice-to-have features like process.env.PROXY.

A mock-up implementation:

const data = await request(URL, {json: true}); const out = {}; data.forEach(...); await writeFileAsync(DEST, JSON.stringify(out, null, 2));

I had thought of using the request module but didn't want to introduce a dependency unnecessarily. Will add as a dev dependency and address this in a follow up PR.

virkt25 requested review from bajtos and raymondfeng as code owners May 31, 2018 00:08

virkt25 self-assigned this May 31, 2018

virkt25 requested review from hacksparrow, jannyHou, b-admike, shimks and dhmlau May 31, 2018 00:10

virkt25 force-pushed the indexer branch from a31f9b8 to a9fc0eb Compare June 1, 2018 03:54

shimks reviewed Jun 1, 2018

View reviewed changes

dhmlau mentioned this pull request Jun 1, 2018

Monthly Milestone - June 2018 #1375

Closed

27 tasks

virkt25 force-pushed the datasource branch 2 times, most recently from a566b7c to 02bec9a Compare June 3, 2018 22:08

virkt25 added this to the June Milestone milestone Jun 3, 2018

virkt25 changed the title ~~[WIP] DataSource Declarative CLI + Boot~~ DataSource Declarative CLI + Boot Jun 3, 2018

bajtos previously requested changes Jun 4, 2018

View reviewed changes

bajtos mentioned this pull request Jun 4, 2018

Add GeoCoder Service to examples/todo #1347

Merged

5 tasks

virkt25 force-pushed the indexer branch from a9fc0eb to a40da75 Compare June 4, 2018 14:30

virkt25 commented Jun 5, 2018

View reviewed changes

virkt25 force-pushed the datasource branch from c64e943 to c4cd7a0 Compare June 5, 2018 05:19

virkt25 changed the base branch from indexer to master June 5, 2018 05:19

raymondfeng reviewed Jun 5, 2018

View reviewed changes

bajtos mentioned this pull request Jun 6, 2018

[Spike]How to modify datasource configuration in integration/acceptance tests #1396

Closed

virkt25 force-pushed the datasource branch from c4cd7a0 to fbe569c Compare June 12, 2018 05:05

bajtos reviewed Jun 12, 2018

View reviewed changes

virkt25 force-pushed the datasource branch from 45e0f45 to aceaa39 Compare June 13, 2018 05:27

raymondfeng reviewed Jun 13, 2018

View reviewed changes

raymondfeng approved these changes Jun 13, 2018

View reviewed changes

b-admike reviewed Jun 13, 2018

View reviewed changes

shimks suggested changes Jun 13, 2018

View reviewed changes

b-admike reviewed Jun 13, 2018

View reviewed changes

shimks reviewed Jun 13, 2018

View reviewed changes

b-admike approved these changes Jun 13, 2018

View reviewed changes

shimks approved these changes Jun 13, 2018

View reviewed changes

virkt25 force-pushed the datasource branch from 205952a to 0bc40ea Compare June 13, 2018 23:18

virkt25 added 5 commits June 13, 2018 19:41

feat(cli): add lb4 datasource command

c737991

Signed-off-by: Taranveer Virk <[email protected]>

fix(repository): accept class and instance for app.datasource

1d7b543

feat(boot): datasource booter

d317d32

refactor(example-todo): use datasource cli generated files and dataso…

3eba183

…urce booters

docs(docs): update docs with datasource cli and booter content

dc70c6c

virkt25 force-pushed the datasource branch from 0bc40ea to dc70c6c Compare June 13, 2018 23:42

virkt25 merged commit 9d48e68 into master Jun 14, 2018

virkt25 deleted the datasource branch June 14, 2018 00:01

bajtos reviewed Jun 14, 2018

View reviewed changes

virkt25 mentioned this pull request Jun 16, 2018

fix(cli): make download-connector-list more robust #1437

Merged

7 tasks

bajtos mentioned this pull request Jun 19, 2018

Update Todo tutorial to load datasources via loopback boot #1440

Closed


		### Synopsis

		Adds new [DataSource] class and config files to a LoopBack application.

DataSource Declarative CLI + Boot #1385

DataSource Declarative CLI + Boot #1385

Conversation

virkt25 commented May 31, 2018 • edited Loading

Checklist

shimks left a 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

virkt25 Jun 5, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virkt25 Jun 13, 2018 • edited Loading

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

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

virkt25 commented Jun 6, 2018 • edited Loading

bajtos commented Jun 6, 2018

bajtos left a 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

virkt25 Jun 13, 2018 • edited Loading

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

raymondfeng left a 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

shimks left a 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

virkt25 commented May 31, 2018 •

edited

Loading

virkt25 Jun 5, 2018 •

edited

Loading

virkt25 Jun 13, 2018 •

edited

Loading

virkt25 commented Jun 6, 2018 •

edited

Loading

virkt25 Jun 13, 2018 •

edited

Loading