feat(boot): bind content of package.json to app context #1764

raymondfeng · 2018-09-26T03:29:19Z

See loopbackio/loopback4-example-shopping#16 (comment)

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

virkt25 · 2018-09-26T14:43:10Z

packages/boot/src/mixins/boot.mixin.ts

@@ -53,6 +55,11 @@ export function BootMixin<T extends Constructor<any>>(superClass: T) {
      this.bind(BootBindings.BOOT_OPTIONS).toDynamicValue(
        () => this.bootOptions,
      );
+
+      this.bind(CoreBindings.APPLICATION_PACKAGE_JSON).toDynamicValue(() => {


Would it not be better for this to be done via a Booter? This is not the intended usage of bootmixin and seems like concerns are being mixed.

I don't mind moving it to a application booter in the future once we figure out our configuration loading story, such as how to deal with env vars.

virkt25

LGTM overall. i'm ok leaving out an ApplicationBooter till we have more stuff to add to it. Just one last comment and this can land imo.

virkt25 · 2018-09-27T20:46:50Z

packages/boot/src/mixins/boot.mixin.ts

+
+      this.bind(CoreBindings.APPLICATION_PACKAGE_JSON).toDynamicValue(() => {
+        const pkgFile = path.resolve(this.projectRoot, 'package.json');
+        return require(pkgFile);


Should we do a try/catch here in case package.json path is resolved incorrectly or the file doesn't exist (not sure why that may happen ... but in case it does).

bajtos

Personally, I would much rather see this functionality implemented as a new booter class.

As far as I understand the current design, BooterMixin is an optional helper that is not required for the booter component to work. Users should be able to use app.component(BooterComponent) and still get fully functional bootstrapper.

bajtos · 2018-10-02T11:04:41Z

packages/boot/test/acceptance/controller.booter.acceptance.ts

@@ -21,6 +23,15 @@ describe('controller booter acceptance tests', () => {

  afterEach(stopApp);

+  it('binds package.json', async () => {


Please move this test to a more appropriate file, maybe create a new test file in test/acceptance directory?

It looks wrong to me to see package.json-related tests inside controller booter acceptance tests.

(I think this is another clue that package.json should be handled by a new booter.)

bajtos · 2018-10-02T11:06:27Z

packages/core/src/application.ts

@@ -192,6 +192,10 @@ export class Application extends Context {
    const instance = this.getSync<Component>(componentKey);
    mountComponent(this, instance);
  }
+
+  public packageJson(pkg: PackageJson) {


I find this name confusing for users and readers.

app.packageJson(/*...*/);

Can we find a better name, for example setApplicationMetadata? (I do realize this is not consistent with other API like app.component or app.controller.)

bajtos · 2018-10-02T11:07:43Z

packages/core/src/application.ts

+/**
+ * Type description for `package.json`
+ */
+export interface PackageJson {


JSON is a serialization format. Can we replace it with something else in the interface name please? How about PackageManifest, PackageMetadata or even ApplicationManifest/ApplicationMetadata?

I think I like ApplicationMetadata most.

bajtos · 2018-10-02T11:09:57Z

packages/core/src/application.ts

+  version: string;
+  description: string;
+  // tslint:disable-next-line:no-any
+  [name: string]: any;


If this data object is typically loaded from JSON, then I think we should restrict the allowed property types to number | string | object | array | null. JSON does not support functions and Dates for example. The current type definition makes it easy to accidentally treat a package.json property as a function and call it.

If this data object is typically loaded from JSON, then I think we should restrict the allowed property types to number | string | object | array | null. JSON does not support functions and Dates for example. The current type definition makes it easy to accidentally treat a package.json property as a function and call it.

This comment hasn't been addressed nor responded to, PTAL.

raymondfeng · 2018-10-03T23:20:15Z

@bajtos I added a application-metadata.booter and updated names. PTAL.

bajtos

The patch looks much better now!

bajtos · 2018-10-05T13:02:51Z

packages/core/src/application.ts

@@ -192,6 +192,16 @@ export class Application extends Context {
    const instance = this.getSync<Component>(componentKey);
    mountComponent(this, instance);
  }
+
+  /**
+   * Set application metadata. By default, the content of `package.json` is


"By defaults" implies to me that package.json metadata is populated by Application constructor, which is not true.

Can we make it more clear please that this metadata is empty by default but @loopback/boot populates is from package.json?

bajtos · 2018-10-05T13:03:25Z

packages/core/src/application.ts

+  version: string;
+  description: string;
+  // tslint:disable-next-line:no-any
+  [name: string]: any;


If this data object is typically loaded from JSON, then I think we should restrict the allowed property types to number | string | object | array | null. JSON does not support functions and Dates for example. The current type definition makes it easy to accidentally treat a package.json property as a function and call it.

This comment hasn't been addressed nor responded to, PTAL.

raymondfeng · 2018-10-06T20:17:02Z

@bajtos Fixed. PTAL.

bajtos

LGTM.

Please double check that the tests are passing after rebasing your changes on top of the latest master.

bajtos · 2018-10-09T08:09:36Z

packages/boot/test/acceptance/controller.booter.acceptance.ts

@@ -33,6 +33,7 @@ describe('controller booter acceptance tests', () => {
  });

  async function getApp() {
+    await sandbox.copyFile(resolve(__dirname, '../fixtures/package.json'));


I think this will not work after the changes I made in #1824) - the package.json file is not going to be copied over from test to dist/test.

I think the easiest solution is to load the package json file from test/fixtures/application.ts:

import * as pkg from './package.json'

raymondfeng requested a review from bajtos as a code owner September 26, 2018 03:29

virkt25 reviewed Sep 26, 2018

View reviewed changes

raymondfeng force-pushed the bind-package-json branch from d8180ef to 30b4dcb Compare September 27, 2018 18:23

virkt25 reviewed Sep 27, 2018

View reviewed changes

raymondfeng mentioned this pull request Oct 1, 2018

feat(cli): add code template for default home page controller #1763

Merged

7 tasks

bajtos reviewed Oct 2, 2018

View reviewed changes

raymondfeng force-pushed the bind-package-json branch from 30b4dcb to 699b622 Compare October 3, 2018 23:19

raymondfeng force-pushed the bind-package-json branch from 699b622 to 083d8d4 Compare October 4, 2018 15:50

bajtos reviewed Oct 5, 2018

View reviewed changes

raymondfeng force-pushed the bind-package-json branch 3 times, most recently from 65ba761 to 9424a11 Compare October 5, 2018 22:04

raymondfeng force-pushed the bind-package-json branch from 9424a11 to f104c29 Compare October 6, 2018 20:17

bajtos approved these changes Oct 9, 2018

View reviewed changes

feat(boot): bind content of package.json to app context

c111fba

raymondfeng force-pushed the bind-package-json branch from f104c29 to c111fba Compare October 9, 2018 22:06

raymondfeng merged commit 82f0ebe into master Oct 9, 2018

raymondfeng deleted the bind-package-json branch October 9, 2018 22:25

bajtos mentioned this pull request Oct 19, 2018

Allow components to contribute start actions #1845

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(boot): bind content of package.json to app context #1764

feat(boot): bind content of package.json to app context #1764

raymondfeng commented Sep 26, 2018 •

edited

Loading

virkt25 Sep 26, 2018

raymondfeng Sep 26, 2018

virkt25 left a comment

virkt25 Sep 27, 2018

bajtos left a comment

bajtos Oct 2, 2018

bajtos Oct 2, 2018

bajtos Oct 2, 2018

bajtos Oct 2, 2018

bajtos Oct 5, 2018

raymondfeng Oct 5, 2018

raymondfeng commented Oct 3, 2018

bajtos left a comment

bajtos Oct 5, 2018

bajtos Oct 5, 2018

raymondfeng commented Oct 6, 2018

bajtos left a comment

bajtos Oct 9, 2018

		@@ -21,6 +23,15 @@ describe('controller booter acceptance tests', () => {

		afterEach(stopApp);

		it('binds package.json', async () => {

feat(boot): bind content of package.json to app context #1764

feat(boot): bind content of package.json to app context #1764

Conversation

raymondfeng commented Sep 26, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virkt25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

raymondfeng commented Oct 3, 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

raymondfeng commented Oct 6, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Sep 26, 2018 •

edited

Loading