chore: clean up extension generator #6158

hacksparrow · 2020-08-20T16:37:46Z

Addresses #5336.

Checklist

DCO (Developer Certificate of Origin) signed in all commits
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

bajtos

Thank you for starting the work in cleaning up the template for extensions. I find your implementation as very repetitive and easy to break if we don't remember to update all places repeating the same string manipulation logic. Please move the string manipulation to the generator.

This of the generator as a controller and the template as a view in MVC pattern. Views should be dump and deal only with rendering - converting data produced by the controller to the output format (HTML in MVC, TypeScript code in our case). Data manipulation and business logic belongs to the controller.

bajtos · 2020-08-21T08:49:18Z

packages/cli/generators/extension/templates/src/component.ts.ejs

@@ -1,9 +1,13 @@
 import {Component, ProviderMap} from '@loopback/core';
+import {<%= project.name.charAt(0).toUpperCase() + project.name.slice(1) %>Bindings} from './keys'


We should not perform case manipulation in the template. Please move this code to the generator and make sure we use the same logic for all places (templates) that need to acces {ProjectName}Bindings.

Suggested change

import {<%= project.name.charAt(0).toUpperCase() + project.name.slice(1) %>Bindings} from './keys'

import {<%= project.bindingsNamespace %>} from './keys'

See how we lb4 app is building appClassWithMixins string for inspiration:

https://github.com/strongloop/loopback-next/blob/4fbe6afb19f689b3a27aa4992377e3993ab62973/packages/cli/generators/app/index.js#L118-L134

bajtos · 2020-08-21T08:50:31Z

packages/cli/generators/extension/templates/src/component.ts.ejs

@@ -1,9 +1,13 @@
 import {Component, ProviderMap} from '@loopback/core';
+import {<%= project.name.charAt(0).toUpperCase() + project.name.slice(1) %>Bindings} from './keys'
+import {<%= project.name.toUpperCase() %>_OPTIONS} from './types';


Similarly here.

Suggested change

import {<%= project.name.toUpperCase() %>_OPTIONS} from './types';

import {<%= project.optionsConstant %>} from './types';

hacksparrow · 2020-08-24T09:38:25Z

Thanks @bajtos I have applied your feedback.

raymondfeng · 2020-08-26T16:33:55Z

@hacksparrow It would be nice to document the extension project layout as #6151 does for applications.

hacksparrow · 2020-08-26T16:34:55Z

docs/site/Creating-components.md

-More often than not, the component may want to offer different value providers
-depending on the configuration. For example, a component providing an email API
-may offer different transports (stub, SMTP, and so on).
+Components can be configured by an app by calling `this.configure()` in its


CHANGE NOTE

Let's keep examples as simple as possible and avoid using concepts and terms which can be overwhelming for beginners.

raymondfeng · 2020-08-26T16:35:50Z

packages/cli/generators/extension/templates/src/types.ts.ejs

@@ -0,0 +1,7 @@
+export interface <%= project.optionsInterface %> {


Let's add some comments here to explain the purpose of <Extension>Options and describe specific properties should be added into the empty body.

raymondfeng · 2020-08-26T16:36:29Z

packages/cli/generators/extension/templates/src/component.ts.ejs

+    @inject(CoreBindings.APPLICATION_INSTANCE)
+    private application: Application,
+    @config()
+    options: <%= project.optionsInterface %> = <%= project.defaultOptions %>,


private options?

raymondfeng · 2020-08-26T16:37:08Z

packages/cli/generators/extension/templates/src/component.ts.ejs


+@bind({tags: {[ContextTags.KEY]: <%= project.bindingsNamespace %>.COMPONENT}})


Let's add some comments for the component.

dhmlau · 2020-08-26T16:40:50Z

docs/site/Extending-LoopBack-4.md

@@ -190,7 +190,23 @@ An application-level component usually contributes:
 ### Learn from existing ones

 - [loopback4-example-log-extension](https://github.com/strongloop/loopback-next/tree/master/examples/log-extension)
+- [@loopback/apiconnect](https://github.com/strongloop/loopback-next/tree/master/extensions/apiconnect)


If we are going to have the list of extensions we created in Component.md, it might be better to reference the list in here. It might be to keep both list updated as we're adding more extensions. :)

Following the acceptance criteria. Would like to hear from @bajtos.

Please use your own judgment. The goal is to have a list of all extensions, to make it easier for our users to find them. I don't mind that much where the list is.

I agree with @dhmlau that we should have only one place where we are listing all components. Keeping multiple lists in sync is too much work and if we don't have automated checks in place, then we will almost certainly to forget to update all places sometime in the future.

dhmlau · 2020-08-26T18:08:43Z

docs/site/Component.md

+Here is a list of components officially created and maintained by the LoopBack
+team.
+
+- [@loopback/apiconnect](https://github.com/strongloop/loopback-next/tree/master/extensions/apiconnect) - An extension for IBM API Connect


Suggested change

- [@loopback/apiconnect](https://github.com/strongloop/loopback-next/tree/master/extensions/apiconnect) - An extension for IBM API Connect

- [@loopback/apiconnect](https://github.com/strongloop/loopback-next/tree/master/extensions/apiconnect) - An extension for integrating with [IBM API Connect](https://www.ibm.com/cloud/api-connect)

dhmlau · 2020-08-26T18:09:34Z

docs/site/Component.md

+
+- [@loopback/apiconnect](https://github.com/strongloop/loopback-next/tree/master/extensions/apiconnect) - An extension for IBM API Connect
+- [@loopback/authentication](https://github.com/strongloop/loopback-next/tree/master/packages/authentication) - A LoopBack component for authentication support
+- [@loopback/authentication-jwt](https://github.com/strongloop/loopback-next/tree/master/extensions/authentication-jwt) - Extension for the prototype of JWT


maybe simply Extension for JWT authentication?

dhmlau · 2020-08-26T18:15:05Z

docs/site/Component.md

@@ -83,6 +83,37 @@ const app = new RestApplication();
 app.component(AuthenticationComponent);
 ```

+## Official components
+
+Here is a list of components officially created and maintained by the LoopBack


I see the list below contains the components from the core (e.g. @loopback/rest) and extensions (e.g. @loopback/apiconnect). Perhaps it's better to separate the 2 categories?

@bajtos what do you say?

I don't mind either way.

hacksparrow · 2020-08-27T07:02:57Z

It would be nice to document the extension project layout as #6151 does for applications.

@raymondfeng let's do that in a follow up task, there are two more nice-to-haves in the acceptance criteria, I will club them together.

bajtos · 2020-08-27T14:09:06Z

packages/cli/generators/extension/index.js

+        this.projectInfo.name.slice(1);
+      this.projectInfo.optionsInterface = `${titleCase}Options`;
+      this.projectInfo.bindingsNamespace = `${titleCase}Bindings`;
+      this.projectInfo.defaultOptions = `DEFAULT_${this.projectInfo.name.toUpperCase()}_OPTIONS`;


Please make sure we are handling multi-word names correctly and consistently with other places that are building PascalCase and camelCase identifiers.

A test case to consider: projectInfo.name is todo-demo. I expect the following strings to be used:

options interface: TodoDemoOptions

bindings namespace: TodoDemoBindings

default options: DEFAULT_TODO_DEMO_OPTIONS

Please check how is lb4 app generator dealing with multi-word app names and apply the same solution for extensions. If the app generators behaves differently from what I described above, then please follow what the app generator is doing.

lb4 app is not doing the upperase-underscore thing (eg: DEFAULT_TODO_DEMO_OPTIONS), generating it this way for extension:

const uppercaseUnderscore = this.projectInfo.name .toUpperCase() .replace(/\W/g, '_'); this.projectInfo.defaultOptions = `DEFAULT_${uppercaseUnderscore}_OPTIONS`;

Rest, updated to be like lb4 app.

bajtos · 2020-08-27T14:10:12Z

packages/cli/generators/project/templates/README.md.ejs

+
+## Installation
+
+Describe the installation steps.


I think the generator can pre-fill this section, the typical instructions is to npm install the package, right?

bajtos · 2020-08-27T14:11:31Z

packages/cli/generators/project/templates/README.md.ejs

+
+## Basic Use
+
+Describe the usage.


Can the generator put the default example we are using in our components? Something along the following lines (replace MyComponent with the real name):

this.configure(MyComponent.COMPONENT).to({ // put the configuration options here }); this.component(MyComponent);

bajtos · 2020-08-27T14:12:56Z

packages/cli/generators/project/templates/README.md.ejs

+
+## License
+
+Specify the license.


I suspect that many people won't bother to edit this section and push it to github & npmjs as-is. I think it's better to leave out the License section completely, rather than have content that's meaningless.

raymondfeng · 2020-08-28T16:32:44Z

packages/cli/generators/extension/templates/src/component.ts.ejs

+  config,
+  ContextTags,
+  CoreBindings,
+  createBindingFromClass,


The import is not used. Please remove or leave it behind a comment.

raymondfeng · 2020-08-28T16:34:45Z

packages/cli/generators/project/templates/README.md.ejs

+as shown below.
+
+```ts
+...


Use // ... instead?

Do we want to include import statements here?

raymondfeng · 2020-08-28T16:34:55Z

packages/cli/generators/project/templates/README.md.ejs

+  // Put the configuration options here
+});
+this.component(<%= project.componentName %>);
+...


raymondfeng · 2020-08-28T16:37:19Z

packages/cli/generators/project/templates/README.md.ejs

+
+```ts
+...
+this.configure(<%= project.componentName %>.COMPONENT).to({


This should be <%= project.bindingsNamespace %>.COMPONENT.

Should we have something like:

const options: <%= project.optionsInterface %> = <%= project.defaultOptions %>; this.configure(<%= project.bindingsNamespace %>.COMPONENT).to(options);

bajtos

Looks mostly good, please address @raymondfeng comments.

I am not sure if the algorithm for building defaultOptions name will produce values consistent with other names (especially in app generator) in all edge cases. This should be easy to fix later, so I guess it's ok to wait until there is a bug report (if there is any problem ever).

bajtos · 2020-08-31T06:45:07Z

docs/site/Creating-components.md

+{% include code-caption.html content="application.ts" %}
+
+```ts
+...


Let's keep code snippets a valid TypeScript please.

Suggested change

...

// ...

Per Raymond's suggestions above. The same comment applies to all other places where you are using ....

dhmlau

LGTM from the documentation perspective. I had one suggestion to include the community extensions as well.

dhmlau · 2020-08-31T14:23:32Z

docs/site/Component.md

+  Resource pooling service for LoopBack 4
+- [@loopback/typeorm](https://github.com/strongloop/loopback-next/tree/master/extensions/typeorm) -
+  Adds support for TypeORM in LoopBack
+


do we want to add the link for community extensions as well? https://loopback.io/doc/en/lb4/Community-extensions.html

raymondfeng

Please squash all commits into one before landing the PR.

hacksparrow · 2020-08-31T15:31:19Z

Sure, thanks.

Clean up the extension generator. Signed-off-by: Yaapa Hage <[email protected]>

bajtos suggested changes Aug 21, 2020

View reviewed changes

hacksparrow force-pushed the cleanup-extension branch 2 times, most recently from 6c8bc9b to 4fd36e3 Compare August 26, 2020 16:22

hacksparrow marked this pull request as ready for review August 26, 2020 16:26

hacksparrow requested review from agnes512, emonddr, jannyHou and raymondfeng as code owners August 26, 2020 16:26

hacksparrow force-pushed the cleanup-extension branch from 4fd36e3 to e45f386 Compare August 26, 2020 16:30

hacksparrow commented Aug 26, 2020

View reviewed changes

raymondfeng reviewed Aug 26, 2020

View reviewed changes

dhmlau reviewed Aug 26, 2020

View reviewed changes

hacksparrow force-pushed the cleanup-extension branch 2 times, most recently from ecf7037 to 7df1e9d Compare August 27, 2020 12:25

bajtos reviewed Aug 27, 2020

View reviewed changes

hacksparrow force-pushed the cleanup-extension branch from c89ee7f to 56517ca Compare August 28, 2020 16:09

raymondfeng reviewed Aug 28, 2020

View reviewed changes

bajtos approved these changes Aug 31, 2020

View reviewed changes

dhmlau approved these changes Aug 31, 2020

View reviewed changes

hacksparrow force-pushed the cleanup-extension branch 2 times, most recently from 0e0ac32 to 6c80af3 Compare August 31, 2020 14:33

raymondfeng approved these changes Aug 31, 2020

View reviewed changes

chore: clean up extension generator

c5e90b2

Clean up the extension generator. Signed-off-by: Yaapa Hage <[email protected]>

hacksparrow force-pushed the cleanup-extension branch from 6c80af3 to c5e90b2 Compare August 31, 2020 15:34

hacksparrow merged commit a4d9b15 into master Aug 31, 2020

hacksparrow deleted the cleanup-extension branch August 31, 2020 15:59

hacksparrow mentioned this pull request Aug 31, 2020

Clean up extension template and docs #5336

Closed

10 tasks

		@@ -1,9 +1,13 @@
		import {Component, ProviderMap} from '@loopback/core';
		import {<%= project.name.charAt(0).toUpperCase() + project.name.slice(1) %>Bindings} from './keys'

	import {<%= project.name.charAt(0).toUpperCase() + project.name.slice(1) %>Bindings} from './keys'
	import {<%= project.bindingsNamespace %>} from './keys'

	import {<%= project.name.toUpperCase() %>_OPTIONS} from './types';
	import {<%= project.optionsConstant %>} from './types';

		@@ -0,0 +1,7 @@
		export interface <%= project.optionsInterface %> {


		@bind({tags: {[ContextTags.KEY]: <%= project.bindingsNamespace %>.COMPONENT}})

	- [@loopback/apiconnect](https://github.com/strongloop/loopback-next/tree/master/extensions/apiconnect) - An extension for IBM API Connect
	- [@loopback/apiconnect](https://github.com/strongloop/loopback-next/tree/master/extensions/apiconnect) - An extension for integrating with [IBM API Connect](https://www.ibm.com/cloud/api-connect)

chore: clean up extension generator #6158

chore: clean up extension generator #6158

Conversation

hacksparrow commented Aug 20, 2020

Checklist

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

hacksparrow commented Aug 24, 2020

raymondfeng commented Aug 26, 2020

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

hacksparrow commented Aug 27, 2020 • 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

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

hacksparrow commented Aug 31, 2020

hacksparrow commented Aug 27, 2020 •

edited

Loading