[OC-28] Cli init allows for --templateType to be npm module #480

nickbalestra · 2017-05-10T15:02:56Z

If the templatetype passed to the init comand is not 'jade' or 'handlebars' (default) but a valid oc-template module, the cli will download it and use it to generate boilerplate.

Example of boilerplate code used by the cli to generate a component

Coded to easily add yarn in the future 😍 (not yet supported)
Don't install template if already available on local dev registry
Use boilerplate code hosted template
add readme into boilerplates
logger dependcy injection
unit tests

A couple of extra notes:
For the moment templates defines "compileDependencies" in their package.json. Those are required and will be installed in order to compile template-specific components and therefore templates to work rely on being initialized through the oc init command. The rationale was to avoid having to install all such dependencies on the registry or in places where are not needed at all. Down-size is that for a component creator point of view the api is not too ergonomic as it won't be possible to simply create a component with the right template type and dependencies to work without running oc init or manually installing compileDependencies. To address this in the near future we might split templates in two modules, a client modules and a compile module.

matteofigus · 2017-05-11T19:38:36Z

I'm not sure what's happening with appveyor. When I click details, it goes to a different PR

matteofigus

First round of reviews. There are many minor things but the overall structure seems ok and I think tests cover the
various scenarios quite well 👍

matteofigus · 2017-05-15T16:51:44Z

src/cli/domain/init-template/blueprint.js

+  const logger = config.logger;
+
+  try {
+    const bluprinting = new Spinner(`Blueprinting component...`);


Can we move this to the shared strings?

matteofigus · 2017-05-15T16:51:56Z

src/cli/domain/init-template/blueprint.js

+
+    fs.writeJsonSync(componentPath + '/package.json', packageContent);
+    bluprinting.stop();
+    logger.log(`${colors.green('✔')} Files created at ${componentPath}`);


same for this one

matteofigus · 2017-05-15T16:52:03Z

src/cli/domain/init-template/blueprint.js

+    logger.log(`${colors.green('✔')} Files created at ${componentPath}`);
+    return callback(null, { ok: true });
+  } catch (error) {
+    return callback(`Blueprinting failed. Please open an issue on ${templatePackage.bugs.url} with the following information: ${error}`);


and this one

matteofigus · 2017-05-15T16:52:17Z

src/cli/domain/init-template/createComponentDir.js

+  creating.start();
+  fs.ensureDirSync(componentPath);
+  creating.stop(true);
+  logger.log(`${colors.green('✔')} Created directory "${componentName}"`);


matteofigus · 2017-05-15T16:55:12Z

src/cli/domain/init-template/index.js

+  createComponentDir(config);
+
+  try {
+    // If template available in the dev registry, generate boilerplate out of its blueprint


Whatever this comment is trying to tell me, I don't understand it :D Can you be more clear about what this is doing?

matteofigus · 2017-05-15T17:00:28Z

src/cli/domain/local.js

-        const pathDir = '../../components/base-component-' + templateType,
-          baseComponentDir = path.resolve(__dirname, pathDir),
-          npmIgnorePath = path.resolve(__dirname, pathDir + '/.npmignore');
+      // LEGACY TEMPLATES


Can we move this to own file like init-legacy-templates.js so that we can make this comment redundant?

matteofigus · 2017-05-15T17:00:59Z

src/cli/facade/init.js

@@ -19,7 +19,7 @@ module.exports = function(dependencies){

    callback = wrapCliCallback(callback);

-    local.init(componentName, templateType, (err) => {
+    local.init(componentName, templateType, { logger: logger }, err => {


{ logger } ?

matteofigus · 2017-05-15T17:02:51Z

test/unit/cli-domain-init-template--createComponentDir.js

+    });
+    it('should correctly invoke ensureDirSync', () => {
+      expect(deps['fs-extra'].ensureDirSync.calledWith(
+        config.componentPath


personal preference, I like repetition on tests to improve readability. I prefer to read that's called with path/to/component over the config, so that if I scroll a lot in the bottom, each tests still tells me what this is doing by looking at it instead of scrolling back up?

im fine either way

matteofigus · 2017-05-15T17:08:00Z

test/unit/cli-domain-init-template--initPackage.js

+  }
+};
+
+const initPackage = injectr('../../src/cli/domain/init-template/initPackage.js', deps, {});


This is a minor one, but I think we usually follow conventions of init-package for filenames. Are you able to bring all the new files in this PR back to follow the same convention?

matteofigus · 2017-05-15T17:08:36Z

test/unit/cli-domain-init-template--utils.js

+    it('should return the unaltered package name if alrady given', () => {
+      expect(getPackageName('oc-template-jade')).to.equal('oc-template-jade');
+      expect(getPackageName('are-we-there-yet-1.1.2.tgz')).to.equal('are-we-there-yet');
+      expect(getPackageName('oc-template-jade@next')).to.equal('oc-template-jade');


well done on testing all of these scenarios

nickbalestra · 2017-07-22T20:57:56Z

@matteofigus I've change pr base so that now this will merge to master (as the templates branch was already been merged). Addressed all the previous points, rebased and improved it a bit further.

matteofigus

This is a second round of review. There may be more to come but I think this is good to start

matteofigus · 2017-07-24T13:28:32Z

src/resources/index.js

+      installCompiler: (compiler, fromLocal) =>
+        `Installing ${compiler} from ${fromLocal ? 'local' : 'npm'}...`,
+      installCompilerSuccess: (template, compiler, version) =>
+        `${colors.green('✔')} Installed ${compiler} [${template.replace(


I think template.replace is not something I would put here. I prefer this to have simple text replacements and this to be done to however calls this (template should already be without -compiler)

matteofigus · 2017-07-24T13:29:44Z

src/cli/domain/init-template/success.js

@@ -0,0 +1,20 @@
+module.exports = (logger, componentName, componentPath) => {


can we use a multiline template literal and actually move this to the resources file?

Cause multiline is space sensitive, it will look a bit ugly on that file. I've commited it so that you can see. I think is ok for now as we need to rethink that part anyway.

matteofigus · 2017-07-24T13:31:14Z

src/cli/domain/init-template/index.js

+  const {
+    componentName,
+    templateType,
+    logger = console,


logger will be sent by the facade, so we don't need to fallback to console

matteofigus · 2017-07-24T13:32:20Z

src/cli/domain/init-template/index.js

+    logger = console,
+    cli = 'npm'
+  } = options;
+  const componentPath = path.join(process.cwd(), componentName);


can we generate componentPath in the facade, and pass it through each call? so that process-cwd() is not repeated in multiple places.

It actually happen in one place. The other calls to process-cwd() are for different matters (local compiler, resolve for path being passed for template). But agree on hoisting it up in the facade. 👍

matteofigus · 2017-07-24T13:33:48Z

src/cli/domain/init-template/init-package.js

+
+module.exports = function(options) {
+  const { cli, componentPath } = options;
+  spawn.sync(cli, ['init', '--yes'], { cwd: componentPath });


what's the point on abstracting cli into a separate variable if we support only npm? I would rather explicitly have npm here and abstract only if we add support to yarn or other tools in the future.

matteofigus · 2017-07-24T14:16:00Z

src/cli/domain/init-template/scaffold.js

+
+    fs.copySync(baseComponentFiles, componentPath);
+
+    const componentPackage = require(componentPath + '/package.json');


I would use readJsonSync as you are requiring something and then overriding its own cache by changing the file immediately after...

matteofigus · 2017-07-24T14:16:42Z

src/cli/domain/init-template/scaffold.js

+    });
+
+    scaffold.stop();
+    // logger.log(strings.messages.cli.scaffoldSuccess(componentPath));


what is this comment?

matteofigus · 2017-07-24T14:19:37Z

src/cli/domain/local.js

-        fs.outputJsonSync(componentPath, component, { spaces: 2 });
-
-        return callback(null, { ok: true });
+        initTemplate({ componentName, templateType, logger }, callback);


initTemplate looks asynchronous to me. So this try/catch should be converted to an async callback check

matteofigus · 2017-07-24T14:20:53Z

src/resources/index.js

@@ -135,9 +138,24 @@ module.exports = {
  },
  messages: {
    cli: {
+      installCompiler: (compiler, fromLocal) =>


can we keep those alphabetic. Perhaps first all lower case (new format) then all upper case (old format to be converted to new)

matteofigus · 2017-07-24T14:23:00Z

src/resources/index.js

+        `${colors.green('✔')} Files created at ${componentPath}`,
+      creatingDir: () => `Creating directory...`,
+      createdDir: dirName =>
+        `${colors.green('✔')} Created directory "${dirName}"`,


I am not sure I like the ${colors.green('✔')} been here. The responsibility of this file should not be to have colors. I would rather create a new method in the logger, like logger.okV('message') which does this for you?

I'll suggest we do this afterward as we rethink/rearchitect the facade part

nickbalestra · 2017-07-24T15:17:50Z

@matteofigus Thanks for the great review/comments.
After our conversation, aside addressing the review, couple of things extras:

move all the logging/ui into the facade - To be done in a follow-up PR
removed support for path being passed for templates.
removed spinner support (so that everything is a bit simpler yet, we can do some follow-up PR when we address the facade/cliUX system)
addressed all of the previous comments

shrinkwrap ora replacement blueprint and error handling shwrinkrap test wip dependency injection for logger fixed init.js tests init-template cleanup

matteofigus

couple of little things. Also, can you qa doing oc init hello jade|handlebars to test if all works without breaking changes compared to the past? (testing that then both dev and publish work as expected)? Thanks

matteofigus · 2017-07-26T17:34:38Z

test/unit/cli-domain-init-template--initPackage.js

+  describe('when invoked', () => {
+    const options = {
+      componentPath: 'path/to/component',
+      cli: 'npm'


I think we can remove this line

matteofigus · 2017-07-26T17:34:52Z

test/unit/cli-domain-init-template--installTemplate.js

+  describe('when invoked', () => {
+    const config = {
+      templateType: 'oc-template-jade',
+      cli: 'npm',


matteofigus · 2017-07-26T17:35:01Z

test/unit/cli-domain-init-template--installTemplate.js

+      templateType: 'oc-template-jade',
+      cli: 'npm',
+      componentPath: 'path/to/component',
+      local: false,


matteofigus · 2017-07-26T17:36:03Z

src/cli/domain/init-template/index.js

+    componentName,
+    logger,
+    componentPath,
+    compiler


matteofigus · 2017-07-26T17:36:10Z

src/cli/domain/init-template/install-template.js

+    templateType,
+    compiler,
+    componentPath,
+    logger


matteofigus · 2017-07-26T17:36:20Z

src/cli/domain/init-template/scaffold.js

+    componentPath,
+    compiler,
+    compilerPath,
+    logger


matteofigus · 2017-07-26T17:36:32Z

src/cli/facade/init.js

+        templateType,
+        logger,
+        componentPath,
+        compiler: `${templateType}-compiler`


nickbalestra · 2017-07-26T20:17:58Z

@matteofigus addressed the reviewed points. Some context for you when you'll review it again:

We weren't doing any validation after the npm install of the compiler. As it could be installed any npm module during that process I've added a validation step as soon as installation succeed .
As we already had that validator built-in into require-template.js I've took it out for better reuse.

I've QA the following scenarios (both with the compiler already installed on the dev registry or not):

oc init myComponent -> will setup oc-template-handlebars (work both init & dev, therefore publish)
oc init myComponent handlebars -> will setup oc-template-handlebars (work both init & dev, therefore publish)
oc init myComponent jade -> will setup oc-template-jade (work both init & dev, therefore publish)
oc init myComponent oc-template-handlebars/jade -> will setup oc-template-handlebars/jade (work both init & dev, therefore publish)

All seems good now

matteofigus · 2017-07-26T21:15:56Z

LGTM. Let's merge and release tomorrow first thing 👍

This was referenced May 11, 2017

Initial version for oc-react-template opencomponents/oc-template-react#1

Merged

oc init to support custom templates #341

Closed

matteofigus closed this May 11, 2017

matteofigus reopened this May 11, 2017

matteofigus changed the title ~~[GPT-537]Cli init allows for --templateType to be npm module~~ [GPT-537] Cli init allows for --templateType to be npm module May 11, 2017

nickbalestra added the templates label May 12, 2017

nickbalestra force-pushed the GPT-537 branch from 9104a8b to fa1d5d9 Compare May 15, 2017 11:13

matteofigus requested changes May 15, 2017

View reviewed changes

nickbalestra mentioned this pull request May 26, 2017

OC templates architecture, status-quo, lessons learned and next steps #494

Closed

nickbalestra force-pushed the templates branch from 9d7c70f to 9e50201 Compare June 23, 2017 12:16

nickbalestra force-pushed the templates branch from d7c1184 to 8f5f03d Compare July 4, 2017 14:15

nickbalestra force-pushed the templates branch 4 times, most recently from 38ea952 to f8101a5 Compare July 21, 2017 09:31

nickbalestra added the wip label Jul 21, 2017

nickbalestra force-pushed the GPT-537 branch from fa1d5d9 to f018f83 Compare July 21, 2017 09:49

nickbalestra changed the base branch from templates to master July 22, 2017 20:23

nickbalestra force-pushed the GPT-537 branch from e59841f to f050cc4 Compare July 22, 2017 20:27

nickbalestra added cli and removed wip labels Jul 22, 2017

nickbalestra force-pushed the GPT-537 branch from f150602 to 09571db Compare July 24, 2017 09:21

matteofigus requested changes Jul 24, 2017

View reviewed changes

nickbalestra force-pushed the GPT-537 branch 2 times, most recently from 8fb76ee to 490fadb Compare July 24, 2017 23:06

cli init now allow for template to support initialization

b6efbb1

shrinkwrap ora replacement blueprint and error handling shwrinkrap test wip dependency injection for logger fixed init.js tests init-template cleanup

Nick Balestra added 18 commits July 26, 2017 18:00

modular init code for CLi

655d280

init-template unit test

6dad932

init-template--utils unit test

4d96cbf

init-template--installTemplate unit test

3a2043a

init-template--creatComponentDir unit test

8258502

init-template--initPackage unit test

ec2703e

using devDependencies for template

4ae8fe4

compileDependencies

8fbc9c1

(options, callback) => {} signature

fe7a0dc

moved messages/erros to resources

fe1c52f

renaming blueprint to scaffold

be4f3bf

drop the need of internal base components, warn about changes

2d8307c

file name conventions

5bfce59

improved init DX, strings, and new non-legacy defaults

4088b7c

shwrinkwrap left over

5e33ba7

correctly indent json during scaffolding

74e691a

review

2579caa

drop support for path, spinner, and various issues addressed

4f25287

nickbalestra force-pushed the GPT-537 branch from 490fadb to 4f25287 Compare July 26, 2017 17:02

matteofigus requested changes Jul 26, 2017

View reviewed changes

fixes and improvements

e033a0d

matteofigus approved these changes Jul 26, 2017

View reviewed changes

matteofigus changed the title ~~[GPT-537] Cli init allows for --templateType to be npm module~~ [OC-28] Cli init allows for --templateType to be npm module Jul 27, 2017

matteofigus merged commit 343900e into master Jul 27, 2017

matteofigus deleted the GPT-537 branch July 27, 2017 08:26

		@@ -0,0 +1,20 @@
		module.exports = (logger, componentName, componentPath) => {


		fs.copySync(baseComponentFiles, componentPath);

		const componentPackage = require(componentPath + '/package.json');

[OC-28] Cli init allows for --templateType to be npm module #480

[OC-28] Cli init allows for --templateType to be npm module #480

Conversation

nickbalestra commented May 10, 2017

matteofigus commented May 11, 2017

matteofigus 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

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

nickbalestra commented Jul 22, 2017

matteofigus 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

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

nickbalestra commented Jul 24, 2017 • edited Loading

matteofigus 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

Choose a reason for hiding this comment

nickbalestra commented Jul 26, 2017

matteofigus commented Jul 26, 2017

nickbalestra commented Jul 24, 2017 •

edited

Loading