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): auto-generate index.ts for exports #1363

Merged
merged 1 commit into from
Jun 4, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions docs/site/todo-tutorial-controller.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,14 @@ logic will live_!

### Create your controller

So, let's create a controller to handle our Todo routes. Inside the
`src/controllers` directory create the following two files:
So, let's create a controller to handle our Todo routes. You can create an empty
Controller using the CLI as follows:

- `index.ts` (export helper)
- `todo.controller.ts`
```sh
lb4 controller
? Controller class name: todo
? What kind of controller would you like to generate? Empty Controller
```

In addition to creating the handler functions themselves, we'll also be adding
decorators that setup the routing as well as the expected parameters of incoming
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './ping.controller';
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me wonder whether it should be included when the app is generated as opposed to adding it on when the controller generator is run. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check the file -- It's in the app generator and this is just the default index.ts that gets generated since we are generating ping.controller in new apps.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ping controller I know about, but I was wondering about whether we should include this index.ts file when the app generator is run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea when the app generator is run there will be a src/controllers/index.ts that will be generated which exports the ping controller.

26 changes: 7 additions & 19 deletions packages/cli/generators/controller/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const ArtifactGenerator = require('../../lib/artifact-generator');
const debug = require('../../lib/debug')('controller-generator');
const inspect = require('util').inspect;
const path = require('path');
const chalk = require('chalk');
const utils = require('../../lib/utils');

// Exportable constants
Expand All @@ -35,7 +36,7 @@ module.exports = class ControllerGenerator extends ArtifactGenerator {
// XXX(kjdelisle): These should be more extensible to allow custom paths
// for each artifact type.

this.artifactInfo.outdir = path.resolve(
this.artifactInfo.outDir = path.resolve(
this.artifactInfo.rootDir,
'controllers',
);
Expand Down Expand Up @@ -185,10 +186,10 @@ module.exports = class ControllerGenerator extends ArtifactGenerator {
// all of the templates!
if (this.shouldExit()) return false;
this.artifactInfo.name = utils.toClassName(this.artifactInfo.name);
this.artifactInfo.filename =
this.artifactInfo.outFile =
utils.kebabCase(this.artifactInfo.name) + '.controller.ts';
if (debug.enabled) {
debug(`Artifact filename set to: ${this.artifactInfo.filename}`);
debug(`Artifact output filename set to: ${this.artifactInfo.outFile}`);
}
// renames the file
let template = 'controller-template.ts.ejs';
Expand All @@ -204,7 +205,7 @@ module.exports = class ControllerGenerator extends ArtifactGenerator {
debug(`Using template at: ${source}`);
}
const dest = this.destinationPath(
path.join(this.artifactInfo.outdir, this.artifactInfo.filename),
path.join(this.artifactInfo.outDir, this.artifactInfo.outFile),
);

if (debug.enabled) {
Expand All @@ -221,20 +222,7 @@ module.exports = class ControllerGenerator extends ArtifactGenerator {
return;
}

end() {
super.end();
if (this.shouldExit()) return false;
// logs a message if there is no file conflict
if (
this.conflicter.generationStatus[this.artifactInfo.filename] !== 'skip' &&
this.conflicter.generationStatus[this.artifactInfo.filename] !==
'identical'
) {
this.log();
this.log(
'Controller %s is now created in src/controllers/',
this.artifactInfo.name,
);
}
async end() {
await super.end();
}
};
54 changes: 54 additions & 0 deletions packages/cli/lib/artifact-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
const BaseGenerator = require('./base-generator');
const debug = require('./debug')('artifact-generator');
const utils = require('./utils');
const updateIndex = require('./update-index');
const path = require('path');
const chalk = require('chalk');
const StatusConflicter = utils.StatusConflicter;

module.exports = class ArtifactGenerator extends BaseGenerator {
Expand All @@ -29,6 +32,10 @@ module.exports = class ArtifactGenerator extends BaseGenerator {
}
this.artifactInfo.name = this.args[0];
this.artifactInfo.defaultName = 'new';
this.artifactInfo.relPath = path.relative(
this.destinationPath(),
this.artifactInfo.outDir,
);
this.conflicter = new StatusConflicter(
this.env.adapter,
this.options.force,
Expand Down Expand Up @@ -102,4 +109,51 @@ module.exports = class ArtifactGenerator extends BaseGenerator {
{globOptions: {dot: true}},
);
}

async end() {
const success = super.end();
if (!success) return false;

let generationStatus = true;
// Check all files being generated to ensure they succeeded
Object.entries(this.conflicter.generationStatus).forEach(([key, val]) => {
if (val === 'skip' || val === 'identical') generationStatus = false;
});

if (generationStatus) {
/**
* Update the index.ts in this.artifactInfo.outDir. Creates it if it
* doesn't exist.
* this.artifactInfo.outFile is what is exported from the file.
*
* Both those properties must be present for this to happen. Optionally,
* this can be disabled even if the properties are present by setting:
* this.artifactInfo.disableIndexUdpdate = true;
*/
if (
this.artifactInfo.outDir &&
this.artifactInfo.outFile &&
!this.artifactInfo.disableIndexUpdate
) {
await updateIndex(this.artifactInfo.outDir, this.artifactInfo.outFile);
// Output for users
this.log(
chalk.green(' update'),
`${this.artifactInfo.relPath}/index.ts`,
);
}

// User Output
this.log();
this.log(
utils.toClassName(this.artifactInfo.type),
chalk.yellow(this.artifactInfo.name),
'is now created in',
`${this.artifactInfo.relPath}/`,
);
this.log();
}

return false;
}
};
23 changes: 23 additions & 0 deletions packages/cli/lib/update-index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// Copyright IBM Corp. 2017,2018. All Rights Reserved.
// Node module: @loopback/cli
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

const path = require('path');
const util = require('util');
const fs = require('fs');
const appendFileAsync = util.promisify(fs.appendFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

appendFileSync is a blocking / synchronous operation. async / await is a promise under the hood even though it appears to be synchronous when written.

From https://hackernoon.com/6-reasons-why-javascripts-async-await-blows-promises-away-tutorial-c7ec10518dd9

Async/await makes asynchronous code look and behave a little more like synchronous code. This is where all its power lies.

In a generator you can call updateIndex without an await ... and have more code below -- and that additional code and continue running without being blocked essentially.

Here's a Node.js article on blocking / non-blocking. https://nodejs.org/en/docs/guides/blocking-vs-non-blocking/


/**
*
* @param {String} dir The directory in which index.ts is to be updated/created
* @param {*} file The new file to be exported from index.ts
*/
module.exports = async function(dir, file) {
const indexFile = path.join(dir, 'index.ts');
if (!file.endsWith('.ts')) {
throw new Error(`${file} must be a TypeScript (.ts) file`);
}
const content = `export * from './${file.slice(0, -3)}';\n`;
await appendFileAsync(indexFile, content);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we explicitly check whether the line exists already? Or is it that too much of an edge case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. My assumption is to let the generator handle that -- within a generator this function is triggered only when the generationStatus is not an error / identical file. So if the file exists this won't be called. -- That said another check doesn't hurt. 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm, I forgot about yeoman's own conflicter system

};
47 changes: 47 additions & 0 deletions packages/cli/test/unit/update-index.unit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright IBM Corp. 2017,2018. All Rights Reserved.
// Node module: @loopback/cli
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

const updateIndex = require('../../lib/update-index');
const assert = require('yeoman-assert');
const path = require('path');
const util = require('util');
const fs = require('fs');
const writeFileAsync = util.promisify(fs.writeFile);

const testlab = require('@loopback/testlab');
const expect = testlab.expect;
const TestSandbox = testlab.TestSandbox;

// Test Sandbox
const SANDBOX_PATH = path.resolve(__dirname, '.sandbox');
const sandbox = new TestSandbox(SANDBOX_PATH);
const expectedFile = path.join(SANDBOX_PATH, 'index.ts');

describe('update-index unit tests', () => {
beforeEach('reset sandbox', () => sandbox.reset());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to just provide sandbox.reset since it isn't async anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried and I get the following error message if I do beforeEach(sandbox.reset):

1) update-index unit tests
       "before each" hook: reset for "creates index.ts when not present":
     TypeError: this.validateInst is not a function
      at Context.reset (packages/testlab/src/test-sandbox.ts:31:27)

Copy link
Contributor

@shimks shimks Jun 1, 2018

Choose a reason for hiding this comment

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

Looks like it has to do with scoping :(.
On the other hand, do you think async () => await sandbox.reset() is cleaner than () => sandbox.reset())? I feel we might as well use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the current format -- it's how we use sandbox.reset in the beforeEach hooks of other tests. Consistency is good IMO


it('creates index.ts when not present', async () => {
await updateIndex(SANDBOX_PATH, 'test.ts');
assert.file(expectedFile);
assert.fileContent(expectedFile, /export \* from '.\/test';/);
});

it('appends to existing index.ts when present', async () => {
await writeFileAsync(
path.join(SANDBOX_PATH, 'index.ts'),
`export * from './first';\n`,
);
await updateIndex(SANDBOX_PATH, 'test.ts');
assert.file(expectedFile);
assert.fileContent(expectedFile, /export \* from '.\/first'/);
assert.fileContent(expectedFile, /export \* from '.\/test'/);
});

it('throws an error when given a non-ts file', async () => {
expect(updateIndex(SANDBOX_PATH, 'test.js')).to.be.rejectedWith(
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to be wrapped in an anonymous function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I was thinking of error assertions. pls ignore

/test.js must be a TypeScript \(.ts\) file/,
);
});
});