Skip to content
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

feat(cli): improve file structure for generated datasources (json/ts) #4060

Merged
merged 2 commits into from
Nov 11, 2019

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Nov 7, 2019

Improve file structure for datasources - extracted from #4051

Triggered by microsoft/TypeScript#34761

See #4051 (review)

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 👈

@raymondfeng raymondfeng requested a review from bajtos as a code owner November 7, 2019 16:31
@raymondfeng raymondfeng changed the title feat(cli): generate datasource json with '.config.json` extension feat(cli): improve file structure for generated datasources (json/ts) Nov 7, 2019
@raymondfeng
Copy link
Contributor Author

raymondfeng commented Nov 7, 2019

My response to #4051 (review):

Two options are proposes so far:

  1. Rename the *.datasource.json files to avoid conflict.
    • *.config.json (implemented by the current PR)
    • *.datasource-config.json (proposed by @bajtos)
    • *.datasource.config.json (I had this convention before *.config.json)
  2. Inline the json config into *.datasource.ts file

I see a 3rd option:

  1. Keep a separate json file for datasource configuration
  2. Improve datasource booter to use app.configure() to register configuration for datasources
  3. In *.datasource.ts files, use @config to inject configuration into the class

@raymondfeng
Copy link
Contributor Author

I'm proposing to rename *.datasource.json to *.datasource.config.json as the short term solution so that we can fully support TypeScript 3.7.x soon.

@raymondfeng raymondfeng force-pushed the improve-datasource-layout branch from 0d7bcdf to aac5a02 Compare November 7, 2019 23:49
@raymondfeng raymondfeng force-pushed the improve-datasource-layout branch from aac5a02 to d3081fe Compare November 8, 2019 00:04
@raymondfeng
Copy link
Contributor Author

@bajtos Please review.

Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Does it consider as a breaking change?
Also, we probably need to update the loopback4-example-shopping accordingly. Thanks.

@raymondfeng
Copy link
Contributor Author

@dhmlau See loopbackio/loopback4-example-shopping#365.

I don't see it as a breaking change as existing code continues to import from *.datasource.json. The PR only impacts how datasource files are generated.

@bajtos
Copy link
Member

bajtos commented Nov 11, 2019

I'm proposing to rename *.datasource.json to *.datasource.config.json as the short term solution so that we can fully support TypeScript 3.7.x soon.

👍

In *.datasource.ts files, use @config to inject configuration into the class

I was thinking about @config too, let's leave that out of scope of this pull request though.

@bajtos
Copy link
Member

bajtos commented Nov 11, 2019

Does it consider as a breaking change?

I don't consider this as a breaking change for the reasons explained by @raymondfeng above.

However, I think we need to clearly communicate this change and explain our users how is it affecting their exiting projects, what is the migration path and how urgent the upgrade is.

I think a short blog post (independent of our milestone blog) would be ideal.

@bajtos bajtos mentioned this pull request Nov 11, 2019
7 tasks
@raymondfeng raymondfeng merged commit 036bcc8 into master Nov 11, 2019
@raymondfeng raymondfeng deleted the improve-datasource-layout branch November 11, 2019 17:08
@dhmlau dhmlau added the CLI label Nov 11, 2019
achrinza added a commit to linkysh/linkysh that referenced this pull request Nov 14, 2019
updated datasource config json for Typescript 3.7 support as per comment at
loopbackio/loopback-next#4060 (comment)

Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants