Move datasource config from JSON to TS files

[bajtos]

For quite some time I was thinking about our current project layout for datasources and the issues it brings:

1. When using connectors from npm in example repositories, we need to modify the static config from the JSON file to replace the connector name with the real connector module.
2. Dynamic configuration of datasources requires changes in the main application (see [Spike] Allow extensions to contribute custom convention for environment-specific operational configuration #1464 (comment)), which is difficult to reason about.
3. DataSource JSON file is a kind of an oddball, all other artifacts (models, repositories, etc.) are using TypeScript files only.

IIRC, we introduced DataSource JSON files because it was the design already used by LB3 and loopback-workspace. We were anticipating that in early future (from the point of view of 2017), we will write tooling to edit DataSource configuration programmatically - somethings that's easier to implement for JSON files rather than TS files.

Clearly, programmatic editing of datasource config didn't become a priority in the last several years and most likely won't become important anytime soon.

Additionally, we already have pretty good tooling for editing TypeScript source code programmatically, we use it to edit model and repository files when scaffolding new relations.

In that light, I feel there is no need to use DataSource JSON files any more and it's better to move the definition of the (default) configuration object to the "main" TypeScript file.

And that's exactly what this pull request is about. I split the changes into three parts to make them easier to review.

1. f591ab4 updates example applications,
2. fdf15c2 updates CLI templates,
3. ff8db3a updates the docs.

Open questions

What's the best way for communicating these change to our users? Should we treat the CLI change as a breaking change? The new version is not going to break existing projects, but it will start emitting different source code for new datasources. Should we provide a migration guide?

UPDATE 2020-04-06

Based on the discussion, I added a migration guide to Datasource-generator.md page and implemented backwards compatibility layer to support both JSON-based and TypeScript-based configuration of datasources.

My intention is to label this change as a backwards-compatible feature triggering a semver-minor release.

Out of scope

While reviewing all things related to DataSources, I found few more opportunities for cleanup. I prefer to leave them out of this PR, but would like to implement them soon after this patch is landed.

1. Update https://github.com/strongloop/loopback4-example-shopping and https://github.com/strongloop/loopback-next/blob/master/docs/site/Inside-Loopback-Application.md to follow the new style
2. Make the static DataSource property name read-only, set it from config.name to avoid duplication.
3. Rework datasources to receive the configuration via @config, so that they can be configured via app.configure(). Update the docs (especially Cloudant & Bluemix guides), make sure to preserve content for the old configuration style (think of existing projects created before the change).
4. Fix datasource files in examples to use the same style as scaffolded by lb4 datasource. Most notably, change them to participate in app life-cycle and disconnect when the application has stopped. Update the docs accordingly.
5. Verify that integration tests instantiating datasource classes directly are stopping them after the tests are over + update the relevant docs (e.g. testing best practices).

(UPDATE 2020-04-24: created a follow-up issue #5220.)

I'd like to get feedback especially from @raymondfeng as our architect and @nabdelgadir as the FA owner of LB4 project conventions.

/cc @strongloop/loopback-next because this change is going to affect most of our users.

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 👈

TODO

Fix the tests (and possibly the implementation) of the following CLI commands to support & test both the new style but also the legacy one.

• lb4 relation
• lb4 repository
• lb4 rest-crud
• lb4 service
• lb4 openapi

[achrinza]

What's the best way for communicating these change to our users?

Other than through the changelogs, we could...

1. Create an "info" alert in the CLI References docs

2. (long-term proposal) Create a page similar to update.angular.io

3. (long-term proposal) Create a read-only announcement channel on Slack for users who want a low-traffic channel (loosely related to chore: improve issue templates #4030 (comment))

Should we treat the CLI change as a breaking change?

As there isn't a clear scope on which parts of the CLI are expected to be consistent, it should be considered a breaking change.

Examples of scopes:

• JSON-based config ingestion using --config
• Files generated by the CLI
• CLI commands (e.g. lb4 app)
• Module API (importing @loopback/cli as a Typesscript module)

[bajtos]

Thank you @achrinza for chiming in!

Create an "info" alert in the CLI References docs

That's what I am leaning towards too. I am thinking about adding a sub-section to https://loopback.io/doc/en/lb4/DataSource-generator.html, where I can explain the difference in the output of lb4 datasource in versions 2.2.1 (or older) and the to-be-current (or newer) versions, and also provide a short migration guide.

As there isn't a clear scope on which parts of the CLI are expected to be consistent, it should be considered a breaking change.

IIUC, you are saying that if we are not sure, then we should treat the change as breaking? Such strategy seems a bit too conservative to me.

My changes are not going to break any existing projects or applications. After the CLI is upgraded, lb4 datasource will emit different files than before, but that's going to affect only newly scaffolded files. All existing datasources will keep working as before.

The question is whether lb4 repository and lb4 service will support both JSON and TS datasources. At the moment, only JSON datasources are supported - that's something I need to fix before this pull request can be landed. I think the fix should preserve support for both (legacy) JSON and (new) TS style, so that users are not forced to migrate their existing datasources at once.

https://github.com/strongloop/loopback-next/blob/e5a48fc82315292878883e53f63ccf197edbc373/packages/cli/lib/utils.js#L571-L615

Do you still think that a semver-major release is needed when the only change is in the newly scaffolded files?

[raymondfeng]

Does discovery of datasources continue to work for related lb4 commands?

[nabdelgadir]

I think the proposal at a high level is good, but +1 to @raymondfeng's comment. It's important to make sure that discovery related CLI commands are compatible.

I am thinking about adding a sub-section to https://loopback.io/doc/en/lb4/DataSource-generator.html, where I can explain the difference in the output of lb4 datasource in versions 2.2.1 (or older) and the to-be-current (or newer) versions, and also provide a short migration guide.

👍 This is what I would've proposed as well.

Should we treat the CLI change as a breaking change?

We can see how the other CLI commands are affected, then decide this.

[bajtos]

Hello 👋 I updated the pull request with the following improvements:

• Rebased on top of the latest master
• Fixed @raymondfeng's review comment in 8c73f9d
• Added migration guide to 203a1b4
• In eb5774b, I fixed lb4 repository tests to use the new style, added one test to verify support for the legacy style, fixed the implementation to support both styles.

I'd like to get your feedback on this increment, @raymondfeng @nabdelgadir @achrinza

What's remaining: fix the tests of the following CLI commands:

• lb4 service
• lb4 rest-crud
• lb4 relation

[raymondfeng]

I think it's time to break down @loopback/cli into one core and a few plugins. Maybe convert it to TypeScript and start to use @loopback/core for extensibilities.

[nabdelgadir]

This increment looks good to me, with one question

[bajtos]

I think it's time to break down @loopback/cli into one core and a few plugins. Maybe convert it to TypeScript and start to use @loopback/core for extensibilities.

Sure, but that's out of scope of this pull request.

Personally, I think we should start with extracting code for working with project artifacts into a new package, something similar to https://github.com/strongloop/loopback-workspace.

See also #844 [CLI] Get rid of Yeoman.

[achrinza]

Since we can now effectively do variable substitution, this may be a good opportunity to port over Specifying database credentials with environment variables into the LoopBack 4 docs.

[bajtos]

Since we can now effectively do variable substitution, this may be a good opportunity to port over Specifying database credentials with environment variables into the LoopBack 4 docs.

Thank you @achrinza for raising this point!

It was indeed one of my goals to make it easier to provide datasource configuration from environment variables.

However, we haven't decided yet what should be the recommended way for handling environment-specific configuration in LB4. There was some discussion in #1464 plus we already have some LB4 documentation in our deployment guides:

• CloudFoundry (legacy IBM Cloud from pre-Kubernetes days): https://loopback.io/doc/en/lb4/Deploying-to-IBM-Cloud.html#step-4-updating-the-application

We don't have any Kubernetes-specific advice, I am not sure if it's because Kubernetes deployments don't need to change datasource configuration or because we didn't consider that aspect when writing the guide.

Either way, I feel that documenting how to use environment variables in datasource config is out of scope of this pull request, the PR is already too large to my taste. Would you mind creating a follow-up issue for the docs you have in mind?

[bajtos]

I pushed few new commits today, I consider the patch ready for final review. I'll clean up the commit history after the changes are approved.

List of recent changes:

• 3283629 address review comment for docs
• 16ed3db migration guide clean-up per review comments
• 41dc940 address review comments in tests
• ffa7c01 update after rebase on master
• 2f2c023 fix lb4 openapi
• 86fff28 update after rebase on master (again)
• 7920ed8 update lb4 relation test fixtures
• 891ab1d preserve backwards compat for lb4 rest-crud
• b9b3d15 preserve backwards compat for lb4 service

The code in tests is not as clean & consistent as I it would like to be, but I have only so much time available for refactoring existing cruft 🤷

@achrinza @nabdelgadir @raymondfeng Do you have any more comments? Does the pull request LGTY now?

[bajtos]

(Investigating test failures now. It looks like the test is not picking positional: true field from the datasource config.)

[bajtos]

ad7732c should fix the build 🤞

[bajtos]

Oh well, looks like snapshots are not the best tool when testing file paths :(

 const config = {
   name: 'petStore',
   connector: 'openapi',
-  spec: 'C:\projects\loopback-next\packages\cli\test\fixtures\openapi\3.0\petstore-expanded.yaml',
+  spec: '../../../fixtures/openapi/3.0/petstore-expanded.yaml',
   validate: false,
   positional: true
 };

[bajtos]

892d733 is fixing the failing snapshot tests on Windows in a way that (I hope) will improve lives of all LB users on Windows

[bajtos]

@achrinza @nabdelgadir @raymondfeng PTAL, does the pull request LGTY now?

[nabdelgadir]

👍

[bajtos]

I have rebased the patch on top of the latest master and squashed the commits. Unless there are any more comments and/or objections, my plan is to go ahead and merge this change as a backwards-compatible feature on Friday April 24th.