fix: allow 'gts clean' to follow tsconfig.json dependency chain #185

jsamar17 · 2018-07-09T18:10:49Z

I wrote and edited the tests to show the new functionality and to catch circular references. This still should be abstracted to effect all commands that look at the contents of tsconfig.json. Also there is no support for compileOnSave yet. fix: #64

intended

kjin

Some initial comments.

kjin · 2018-07-09T18:17:43Z

src/clean.ts

@@ -30,6 +30,7 @@ export async function clean(options: Options): Promise<boolean> {
  const tsconfig = (await getTSConfig(options.targetRootDir)) as TSConfig;
  if (tsconfig.compilerOptions && tsconfig.compilerOptions.outDir) {
    const outDir = tsconfig.compilerOptions.outDir;
+


Since there are no logical changes to this file, can you keep the original?

Unless I'm mistaken, it seems like this file wasn't reverted to the original.

kjin · 2018-07-09T18:19:28Z

src/util.ts

@@ -15,6 +15,7 @@
 */

 import * as fs from 'fs';
+import {access, read} from 'fs/promises';


We need to support Node 6 and up, and I think 'fs/promises' is only available in Node 10, so I don't think we can use it.

That being said, it seems like these fields are unused, so maybe we can remove this import entirely.

kjin · 2018-07-09T18:28:22Z

src/util.ts

+  let contents = JSON.parse(json);
+
+  const readArr = ['tsconfig.json'];
+  if (contents['extends']) {


For known properties of an object, prefer x.y over x['y'].

kjin · 2018-07-09T19:17:16Z

src/util.ts

@@ -38,12 +39,77 @@ export function nop() {
 /**
 * Find the tsconfig.json, read it, and return parsed contents.
 * @param rootDir Directory where the tsconfig.json should be found.
+ * If the tsconfig.json file has an "extends" field hop down the dependency tree until it ends


I believe the /** ... */ comments specifically annotate getTSConfig. Could you move ConfigFile to preserve this?

kjin · 2018-07-09T19:17:31Z

src/util.ts

 */
+
+
+export interface ConfigFile {


Could you add JSDocs for ConfigFile?

kjin · 2018-07-09T19:28:51Z

src/util.ts

+        await getExtension(contents['extends'], customReadFilep, readArr);
+    contents = combineTSConfig(extension, contents);
+  }
+  return Promise.resolve(contents);


We can just return contents. The return type Promise<ConfigFile> is already guaranteed because this is an async function (async function always returns a Promise, no matter what).

kjin · 2018-07-09T20:12:46Z

src/util.ts

+    filePath: string, customReadFilep: ReadFileP,
+    readFiles: string[]): Promise<ConfigFile> {
+  customReadFilep = customReadFilep || readFilep;
+  if (!filePath) {


This check isn't necessary.

Discussed offline as to why. In general, typechecking allows us to forgo the check as long as the function's not user-facing. (User in this case is anyone who imports this file.)

kjin

...and some more comments. Great test cases!

src/util.ts

kjin · 2018-07-09T20:50:36Z

src/util.ts

+        await getExtension(contents['extends'], customReadFilep, readFiles);
+    contents = combineTSConfig(nextFile, contents);
+  }
+  // console.log(contents);


Did you forget to remove this line? 😄

kjin · 2018-07-09T20:52:46Z

src/util.ts

+    throw new Error('Undefined passed in');
+  }
+  if (readFiles.indexOf(filePath) !== -1) {
+    // did I throw the error correctly?


Yep!

Maybe as part of the error message, I would suggest adding more details about the error? Like the formatted contents of readFiles, for example.

kjin · 2018-07-09T22:18:12Z

src/util.ts

+    // did I throw the error correctly?
+    throw new Error('Circular Reference Detected');
+  }
+  readFiles.push(filePath);


filePath may be relative to the location of tsconfig.json. This might result in issues if say, tsconfig.json depends on foo/tsconfig.bar.json which depends on tsconfig.json: does that last file refer to ./tsconfig.json or ./foo/tsconfig.json?

kjin · 2018-07-09T22:19:40Z

src/util.ts

 }
+
+async function getExtension(


I would consider calling this getBase instead, because the the config file at filePath overrides the config that it extends, not the other way around.

kjin · 2018-07-09T22:31:44Z

src/util.ts

+  return Promise.resolve(contents);
+}
+
+// the inherited config file overwrites the base config file's "files",


Can you move this comment to the lines for which it's relevant? Which, as of this version is around 110.

kjin · 2018-07-09T22:33:24Z

test/test-util.ts

@@ -31,4 +31,74 @@ test('get should parse the correct tsconfig file', async t => {
  t.deepEqual(contents, FAKE_CONFIG);
 });

+
+
+// TODO: test for error due to circular reference


This TODO can be removed now, right?

kjin · 2018-07-09T22:33:59Z

test/test-util.ts

+test('should throw an error if it finds a circular reference', async t => {
+  const FAKE_DIRECTORY = '/some/fake/directory';
+  const FAKE_CONFIG = {files: ['b'], extends: 'CONFIG1'};
+  const CONFIG1 = {extends: 'CONFIG2'};


I would suggest calling these FAKE_CONFIG2, etc.

kjin · 2018-07-09T22:35:08Z

test/test-util.ts

+    files: ['b'],
+    extends: 'CONFIG1'
+  };
+  const CONFIG1 = {include: ['/stuff/*'], extends: 'CONFIG2'};


Can you add exclude and compilerOptions to demonstrate that (1) overwriting happens correctly?

kjin · 2018-07-09T22:36:42Z

test/test-util.ts

+
+
+
+// TODO: test for circular reference error with symlinks


I think we don't necessarily need to test this if other comments are addressed. This is because even with symlinks, we are keeping a cache of "seen" config files, and having symlinks doesn't change the fact that a file will eventually be "seen" twice if it's part of a cycle.

files`

codecov-io · 2018-07-11T18:43:02Z

Codecov Report

Merging #185 into master will increase coverage by 0.15%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
+ Coverage   98.37%   98.52%   +0.15%     
==========================================
  Files          12       12              
  Lines         737      814      +77     
  Branches       61       65       +4     
==========================================
+ Hits          725      802      +77     
  Misses         12       12

Impacted Files	Coverage Δ
src/util.ts	`100% <100%> (ø)`	⬆️
test/test-util.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7c75ee...380a100. Read the comment docs.

kjin · 2018-07-11T20:46:22Z

src/util.ts

 import * as path from 'path';
 import pify from 'pify';
 import rimraf from 'rimraf';
+import {file} from 'tmp';


Because file is so broad in its meaning, I would suggest import * as tmp so as to be more specific. tmp.file(...) is more meaningful than file(...)

This module isn't used in this file so I removed it

kjin · 2018-07-11T20:48:52Z

src/util.ts

-    // did I throw the error correctly?
-    throw new Error('Circular Reference Detected');
+    throw new Error(
+        'Circular reference in ' + readFiles[readFiles.indexOf(filePath)]);


I suggest using the following string interpolation syntax:

const x = 'hello' console.log(`${x}, world`) // prints "hello, world"

kjin · 2018-07-11T20:49:58Z

src/util.ts

+ * An interface containing the high level data fields present in Config Files
+ */
+


Nit: Remove empty line between docs for X and X itself.

kjin · 2018-07-11T20:50:44Z

src/util.ts

Nit: Add a line between a class/interface definition and the docs for the next definition. (I think this is also true of getBase)

kjin · 2018-07-11T20:51:10Z

src/util.ts

-// the inherited config file overwrites the base config file's "files",
-// "include", and "exclude" fields and combines compiler options
+/**
+ * takes in 2 config files


Can you write full sentences in JSDocs? Here as well as in @param parameter descriptions.

kjin · 2018-07-11T20:52:06Z

src/util.ts

-    'exclude': [],
-    'extends': {}
-  };
+  const result = {'compilerOptions': {}, 'extends': {}};


Why is extends one of the fields here? It seems like it's guaranteed to be deleted later.

If it's there for type definition purposes, I would annotate result to be of type ConfigFile.

If combineTSConfig is called that means that a file was contains an extends field. If the extends field is not removed the program will create an infinite loop. I updated it so that it is now part of the ConfigFile definition.

kjin · 2018-07-11T20:54:11Z

src/clean.ts

@@ -30,6 +30,7 @@ export async function clean(options: Options): Promise<boolean> {
  const tsconfig = (await getTSConfig(options.targetRootDir)) as TSConfig;
  if (tsconfig.compilerOptions && tsconfig.compilerOptions.outDir) {
    const outDir = tsconfig.compilerOptions.outDir;
+


Unless I'm mistaken, it seems like this file wasn't reverted to the original.

kjin · 2018-07-11T22:09:57Z

src/util.ts

-
-  const tsconfigPath = path.join(rootDir, 'tsconfig.json');
-
+  const readArr = [''];


Consider making this a Set.

kjin · 2018-07-11T22:11:06Z

test/test-util.ts

@@ -97,8 +97,86 @@ test('should follow dependency chain caused by extends files', async t => {
 });


+test('should throw an error if it finds a circular reference', async t => {


Is this the same test as above?

yes, I removed the redundant test

kjin · 2018-07-11T22:15:11Z

test/test-util.ts

+    });
+
+test(
+    'When a file, all filepaths should be relative to the config file currently being read',


The first part of this sentence seems to be missing a predicate?

ah yes, It should be reading

kjin · 2018-07-11T22:16:23Z

test/test-util.ts

+});
+
+test(
+    'when a file contains an extends field, the base file is loaded first then overridden by the inherited files',


This seems like it is already covered in a test above. (should follow dependency chain caused by extends files)

The difference is that in the first test each config file has a different part of the data and no overwriting is happening, just combining the different fields. This test actually looks to make sure properties are being overwritten correctly. If that is redundant we can remove one

I see, makes sense.

kjin · 2018-07-13T16:42:06Z

cc/ @jplaisted

kjin

Almost there!

kjin · 2018-07-13T19:05:16Z

src/util.ts

+  filePath = path.resolve(currentDir, filePath);
+
+  if (readFiles.has(filePath)) {
+    throw new Error('Circular reference in ${filePath}');


For posterity, can you include a comment on why we throw here (rather than give up and return immediately)?

kjin · 2018-07-13T19:07:09Z

src/util.ts

+ * @param inherited is then loaded and overwrites base
+ */
+function combineTSConfig(base: ConfigFile, inherited: ConfigFile): ConfigFile {
+  const result: ConfigFile = {'compilerOptions': {}};


Nit: Remove '' around compilerOptions.

kjin · 2018-07-13T19:07:44Z

src/util.ts

+}
+
+/**
+ * An interface containing the high level data fields present in Config Files


Nit: top level is more characteristic here.

kjin · 2018-07-13T19:09:13Z

test/test-util.ts

+      configPath: string, encoding: string): Promise<string> {
+    switch (configPath) {
+      case '/some/fake/directory/FAKE_CONFIG1':
+


I think I missed this before. What is this case for? Is this falling through to the next case?

(Here and also in the next test)

Similar to a symlink the file FAKE_CONFIG1 has 2 addresses /some/fake/directory/tsconfig.json and /some/fake/directory/FAKE_CONFIG1. I could edit this out and make the circular references point back to /some/fake/directory/tsconfig.json I kept this in all the tests for consistancy but I can remove it

kjin · 2018-07-13T19:09:52Z

test/test-util.ts

+    });
+
+test(
+    'When reading a file, all filepaths should be relative to the config file currently being read',


Nit: Make this lowercase.

…owing an error

kjin

As we discussed in person, we can use a function factory for fakeReadFilep.

kjin · 2018-07-16T23:52:43Z

test/test-util.ts

-import {getTSConfig} from '../src/util';
+import {ConfigFile, getTSConfig} from '../src/util';
+
+


Can you make a one line break here instead of three.

kjin · 2018-07-16T23:53:42Z

test/test-util.ts

+ * @param myMap contains a filepath as the key and a ConfigFile object as the
+ * value. The fake promisified readFile function has the same interface of
+ * fs.readFile and takes in a
+ * @param configPath and returns a corresponding ConfigFile object or throws an


This @param annotation is a little misleading, since it's a parameter of the returned function, not this one. I think it's acceptable to just say that the return value has the same interface as a promisifed fs.readFile function.

kjin

LGTM when CI passes 👍

jsamar17 added 6 commits July 3, 2018 16:06

added changes to src/util

d4b44ef

fixed naming, somewhat

f1541dc

Cleaned up code

f4ddcf0

Wrote tests for circular cases and dependency. Code is now working as

2eb69c8

intended

Fixed formatting

60b06af

minor formatting and clean up

005b229

googlebot added the cla: yes label Jul 9, 2018

kjin self-requested a review July 9, 2018 18:12

jsamar17 changed the title ~~Issue64~~ fix: Allowed 'gts clean' to follow tsconfig.json dependency chain Jul 9, 2018

kjin reviewed Jul 9, 2018

View reviewed changes

jsamar17 added 3 commits July 11, 2018 10:22

Working on implementing comments from PR

f3e348a

Added functionality and tests to view relative filepaths in tsconfig

bd17d2e

files`

Cleaned up code and minor fixes

16d99ec

kjin reviewed Jul 11, 2018

View reviewed changes

jsamar17 added 13 commits July 11, 2018 15:31

Removed 'tmp' module since it is not being used

b73143f

reverted clean.ts

942a10f

fixed grammer mistakes

c18b7c9

removed TODO

f854ac6

Added newline at end of file

a8150d5

Formatting Fixes

ebfb5e4

More formatting

bccad77

Added newline

190155d

added newline

0cb52e3

Formatting

e22ff6d

Changed readFiles to be a Set

7a79b5c

Formatting

a10d8f6

Minor typo

ced15c4

Removed redundant test

48e8584

Fixed type

edf907a

kjin reviewed Jul 13, 2018

View reviewed changes

kjin changed the title ~~fix: Allowed 'gts clean' to follow tsconfig.json dependency chain~~ fix: allow 'gts clean' to follow tsconfig.json dependency chain Jul 16, 2018

jsamar17 added 7 commits July 16, 2018 10:05

docs: added comment explaining the reason for circular references thr…

c8f2f25

…owing an error

Nit: removed quotes

81512ec

Minor fixes

ebc8548

Ran npm run format

33840ba

Cleaned up functions usings maps

91aa207

Minor syntax fixes

de0a934

Removed some extra newlines

4daafe8

kjin reviewed Jul 16, 2018

View reviewed changes

jsamar17 added 3 commits July 16, 2018 16:13

Added function factory to abstract function creation

52431c8

Added JSdoc for createFakeReadFilep

1ea22ce

Edited JSdoc

c68f2ad

kjin reviewed Jul 16, 2018

View reviewed changes

jsamar17 added 3 commits July 16, 2018 16:56

Minor doc changes

67604dd

Added a test and error handling for nonexistant files

6e12f36

Fixed spelling error

f968189

kjin approved these changes Jul 17, 2018

View reviewed changes

Merge branch 'master' into issue64

380a100

kjin merged commit d0d430d into google:master Jul 17, 2018

		* An interface containing the high level data fields present in Config Files
		*/


		const tsconfigPath = path.join(rootDir, 'tsconfig.json');

		const readArr = [''];

		@@ -97,8 +97,86 @@ test('should follow dependency chain caused by extends files', async t => {
		});


		test('should throw an error if it finds a circular reference', async t => {

		import {getTSConfig} from '../src/util';
		import {ConfigFile, getTSConfig} from '../src/util';

fix: allow 'gts clean' to follow tsconfig.json dependency chain #185

fix: allow 'gts clean' to follow tsconfig.json dependency chain #185

Conversation

jsamar17 commented Jul 9, 2018 • edited

kjin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 11, 2018 • edited

Codecov Report

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

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

kjin commented Jul 13, 2018

kjin 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

kjin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjin left a comment

Choose a reason for hiding this comment

jsamar17 commented Jul 9, 2018 •

edited

codecov-io commented Jul 11, 2018 •

edited