feat(cli): auto-generate index.ts for exports #1363

virkt25 · 2018-05-25T04:27:54Z

This is part of the work I did for the Boot Spike. lb4 datasource makes use of the index generation so decided to just deliver this as it's own feature.

fix #1127

UPDATE: MAY 31

The PR no longer re-generated index.ts -- If already present, it adds a line at the end. If it doesn't exist in the folder it creates it and adds the file to to it.
Auto-generation can be skipped as documented in code

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

shimks

👏👏👏 Small QoL comments

shimks · 2018-05-25T16:27:30Z

packages/cli/generators/controller/index.js

@@ -235,6 +235,7 @@ module.exports = class ControllerGenerator extends ArtifactGenerator {
        'Controller %s is now created in src/controllers/',
        this.artifactInfo.name,
      );
+      super.updateIndexFile();


Should this be returned since it's a promise? It might not matter since it's the last thing being done here, but I think it makes sense to return it for the sake of using a promise

Also, shouldn't updateIndexFile be called in ArtifactGenerator since most artifacts seem to use index.ts?

shimks · 2018-05-25T16:32:41Z

packages/cli/lib/make-index.js

+  const outFile = `${dir}/index.ts`;
+  debug(`outFile => ${outFile}`);
+
+  const writeFileAsync = util.promisify(fs.writeFile);


Is there any reason as to why we don't use fs.writeFileSync?

shimks · 2018-05-25T16:42:13Z

packages/cli/lib/artifact-generator.js

+
+  async updateIndexFile() {
+    await index(this.artifactInfo.outDir, {
+      prefix: `.${this.artifactInfo.type}`,


It's weird to me to have to . as part of prefix. I think it's better for . to be inferred when setting globPattern in make-index.js

bajtos

I have few high-level concerns/comments:

(1) Is there a reasonably-easy way how to test make-index?

(2) Is it a good idea to force index.ts file to be auto-generated? What if app developers decide to add more files, perhaps in a deeper directory structure, and want to export only subset of entities from those additional files?

Instead of generating the whole file, can we simply append new export * from lines at the bottom?

bajtos · 2018-05-29T08:40:23Z

examples/todo/src/controllers/index.ts

-// Copyright IBM Corp. 2017,2018. All Rights Reserved.
-// Node module: @loopback/example-todo
-// This file is licensed under the MIT License.
-// License text available at https://opensource.org/licenses/MIT


Shouldn't the example files preserve license headers? (I understand the files generated by lb4 do not come with license headers.)

bajtos · 2018-05-29T08:41:31Z

packages/cli/.prettierrc

@@ -1,6 +0,0 @@
-{
-  "bracketSpacing": false,


What is the reason for removing .prettierrc from this project? I am not against such change per se, but would like to better understand the reasoning. It may be better to make this change in a standalone pull request.

The reason was that this .prettierrc uses different formatting that the top-level one we use for the project and I noticed that and removed it. I've reverted this change and will make a new PR for this.

bajtos · 2018-05-29T08:47:50Z

packages/cli/lib/format.js

+// This file is licensed under the MIT License.
+// License text available at https://opensource.org/licenses/MIT
+
+const prettier = require('prettier');


Cross-posting item 8 from #441 (comment):

Formatting of generated code: it would be great if our tooling was able to pick up prettier version and configuration from the project we are running in and format the generated code accordingly.

Imagine a user is using prettier in their project (as scaffolded by lb4 app), but changed prettier config to use double-quotes and tabs. If we don't honor their style, they will have to run npm run lint:fix every time they add a new model or update auto-generated TypeScript definitions.

It is also important to run prettier from node_modules of the target project, because different prettier version have variations in how they format the same piece of code and we cannot get all users to have exactly the same version of prettier as we use in LB4 tooling.

It may be enough to add ${this.destinationPath()}/node_modules to the list of directories where Node.js looks for modules. See how Mocha handles that (link):

module.paths.push(cwd, join(cwd, 'node_modules'));

I am ok to leave these two improvements out of scope of this initial pull request as long as there is a follow-up issue (or two?) to ensure we fix this problem as part of the DP3 Boot epic.

bajtos · 2018-05-29T08:48:36Z

packages/cli/lib/make-index.js

+    nested: true,
+    prefix: '',
+    commentHeader: `This is an auto-generated file. DO NOT EDIT.`,
+    commentFooter: `This is an auto-generated file. DO NOT EDIT.`,


Is there any particular reason for using backticks (template literals) instead of regular single-quotes?

bajtos · 2018-05-29T08:50:19Z

packages/cli/lib/make-index.js

+  files.forEach(file => {
+    const relPath = `export * from './${path
+      .relative(dir, file)
+      .slice(0, -3)}';`;


Please extract the expression computing the actual path outside of the template literal and save the result to a variable.

const relativePath = path.relative(dir, file).slice(0, -3); const exportLine = `export * from '${relativePath}';`; lines.push(relPath);

bajtos · 2018-05-29T08:56:22Z

packages/cli/lib/make-index.js

+  debug(`outFile => ${outFile}`);
+
+  const writeFileAsync = util.promisify(fs.writeFile);
+  await writeFileAsync(outFile, code);


This will break incremental builds, because every run of make-index is going to change the modification time of index.ts to the current date, even in cases where the file content was not changed at all.

Proposed solution:

Read the current file content and if the old content is the same as the new content, then don't write anything. I believe this is what prettier is doing too.

Set mtime of the written file to match the latest mtime of files we were generating index from. I.e. for each file in dir (excluding index.ts), obtain mtime value and then pick the maximum value of all mtimes.

I think the second item is not enough on its own, because file watchers like tsc --watch, fs-events, etc. listen for write events and ignore mtime.

The first item may is most likely good enough, because typically the index.ts file would be updated shortly after the last non-index file was added/updated.

I am ok to leave these two improvements out of scope of this initial pull request as long as there is a follow-up issue to ensure we fix this problem as part of the DP3 Boot epic.

bajtos · 2018-05-29T08:57:21Z

packages/cli/lib/make-index.js

+  const comments = comment.split('\n');
+  let normalizedComment = '';
+
+  // For each comment line -- if it's a comment we


if it's a comment we

We do what? Looks like you accidentally forgot to finish the sentence here.

shimks

Looks good at high-level. Couple of thoughts

shimks · 2018-06-01T14:34:13Z

packages/cli/generators/app/templates/src/controllers/index.ts.ejs

@@ -0,0 +1 @@
+export * from './ping.controller';


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?

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.

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

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

shimks · 2018-06-01T14:38:00Z

packages/cli/lib/update-index.js

+const path = require('path');
+const util = require('util');
+const fs = require('fs');
+const appendFileAsync = util.promisify(fs.appendFile);


Why not use this instead? https://nodejs.org/api/fs.html#fs_fs_appendfilesync_path_data_options

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/

shimks · 2018-06-01T14:38:25Z

packages/cli/lib/update-index.js

+    throw new Error(`${file} must be a TypeScript (.ts) file`);
+  }
+  const content = `export * from './${file.slice(0, -3)}';\n`;
+  await appendFileAsync(indexFile, content);


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

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. 🤷‍♂️

Nvm, I forgot about yeoman's own conflicter system

shimks · 2018-06-01T14:40:24Z

packages/cli/test/unit/update-index.unit.js

+const expectedFile = path.join(SANDBOX_PATH, 'index.ts');
+
+describe('update-index unit tests', () => {
+  beforeEach('reset sandbox', () => sandbox.reset());


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

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)

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.

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

shimks · 2018-06-01T14:40:50Z

packages/cli/test/unit/update-index.unit.js

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


needs to be wrapped in an anonymous function

I think I was thinking of error assertions. pls ignore

fix #1127 Signed-off-by: Taranveer Virk <[email protected]>

virkt25 requested review from bajtos, raymondfeng and shimks as code owners May 25, 2018 04:27

virkt25 added needs-priority and removed needs-priority labels May 25, 2018

virkt25 changed the title ~~Indexer~~ feat(cli): auto-generate index.ts for exports May 25, 2018

virkt25 requested a review from a team May 25, 2018 04:28

virkt25 self-assigned this May 25, 2018

virkt25 added this to the May Milestone milestone May 25, 2018

shimks approved these changes May 25, 2018

View reviewed changes

virkt25 force-pushed the indexer branch 2 times, most recently from 19ee9fb to 6932169 Compare May 26, 2018 04:39

bajtos suggested changes May 29, 2018

View reviewed changes

bajtos mentioned this pull request May 29, 2018

[CLI] Generating index.ts in each models/repositories/controllers automatically #1127

Closed

virkt25 force-pushed the indexer branch from 6932169 to a31f9b8 Compare May 30, 2018 19:00

dhmlau mentioned this pull request May 31, 2018

Monthly Milestone - June 2018 #1375

Closed

27 tasks

virkt25 force-pushed the indexer branch from a31f9b8 to a9fc0eb Compare June 1, 2018 03:54

shimks reviewed Jun 1, 2018

View reviewed changes

virkt25 modified the milestones: May Milestone, June Milestone Jun 2, 2018

virkt25 mentioned this pull request Jun 3, 2018

DataSource Declarative CLI + Boot #1385

Merged

13 tasks

bajtos approved these changes Jun 4, 2018

View reviewed changes

feat(cli): auto-generate / update index.ts for exports

a40da75

fix #1127 Signed-off-by: Taranveer Virk <[email protected]>

virkt25 force-pushed the indexer branch from a9fc0eb to a40da75 Compare June 4, 2018 14:30

shimks approved these changes Jun 4, 2018

View reviewed changes

virkt25 merged commit 2998363 into master Jun 4, 2018

bajtos deleted the indexer branch June 5, 2018 07:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): auto-generate index.ts for exports #1363

feat(cli): auto-generate index.ts for exports #1363

virkt25 commented May 25, 2018 •

edited

Loading

shimks left a comment

shimks May 25, 2018

shimks May 25, 2018

shimks May 25, 2018

shimks May 25, 2018

bajtos left a comment

bajtos May 29, 2018

bajtos May 29, 2018

virkt25 Jun 1, 2018

bajtos May 29, 2018

bajtos May 29, 2018

bajtos May 29, 2018

bajtos May 29, 2018

bajtos May 29, 2018

bajtos May 29, 2018

shimks left a comment •

edited

Loading

shimks Jun 1, 2018

virkt25 Jun 1, 2018

shimks Jun 1, 2018

virkt25 Jun 3, 2018

shimks Jun 1, 2018

virkt25 Jun 1, 2018

shimks Jun 1, 2018

virkt25 Jun 1, 2018

shimks Jun 1, 2018

shimks Jun 1, 2018

virkt25 Jun 1, 2018

shimks Jun 1, 2018 •

edited

Loading

virkt25 Jun 1, 2018

shimks Jun 1, 2018

virkt25 Jun 1, 2018

shimks Jun 1, 2018

feat(cli): auto-generate index.ts for exports #1363

feat(cli): auto-generate index.ts for exports #1363

Conversation

virkt25 commented May 25, 2018 • edited Loading

Checklist

shimks 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimks left a comment • 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

Choose a reason for hiding this comment

shimks Jun 1, 2018 • 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

virkt25 commented May 25, 2018 •

edited

Loading

shimks left a comment •

edited

Loading

shimks Jun 1, 2018 •

edited

Loading