Repository: oclif/core
Pull request #147 (2021-04-23 20:44:25)

refactor: globals and duplicated code
Consolidate global access to an oclif settings file. This also moves the columns to the oclif object which is a breaking change with oclif@1.

Consolidate the screen code into a single file, rather in each package as it was in the old oclif packages.


Patch:
diff --git a/.gitignore b/.gitignore
index ccd623f..bb0a634 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,6 +1,7 @@
 *-debug.log
 *-error.log
 /.nyc_output
+/.vscode
 /coverage
 /coverage.lcov
 /lib
diff --git a/src/config/config.ts b/src/config/config.ts
index 3946478..7936bf2 100644
--- a/src/config/config.ts
+++ b/src/config/config.ts
@@ -15,6 +15,7 @@ import * as Plugin from './plugin'
 import {Topic} from '../interfaces/topic'
 import {tsPath} from './ts-node'
 import {compact, flatMap, loadJSON, uniq} from './util'
+import {isProd} from '../util'
 
 // eslint-disable-next-line new-cap
 const debug = Debug()
@@ -461,7 +462,7 @@ export class Config implements IConfig {
   }
 
   protected get isProd() {
-    return process.env.NODE_ENV !== 'development'
+    return isProd()
   }
 }
 
diff --git a/src/config/plugin.ts b/src/config/plugin.ts
index 979781d..b60a369 100644
--- a/src/config/plugin.ts
+++ b/src/config/plugin.ts
@@ -12,6 +12,7 @@ import {PJSON} from '../interfaces/pjson'
 import {Topic} from '../interfaces/topic'
 import {tsPath} from './ts-node'
 import {compact, exists, flatMap, loadJSON, mapValues} from './util'
+import {isProd} from '../util'
 
 const _pjson = require('../../package.json')
 
@@ -112,7 +113,7 @@ export class Plugin implements IPlugin {
     this.name = this.pjson.name
     const pjsonPath = path.join(root, 'package.json')
     if (!this.name) throw new Error(`no name in ${pjsonPath}`)
-    if (process.env.NODE_DEV === 'development' && !this.pjson.files) this.warn(`files attribute must be specified in ${pjsonPath}`)
+    if (!isProd() && !this.pjson.files) this.warn(`files attribute must be specified in ${pjsonPath}`)
     // eslint-disable-next-line new-cap
     this._debug = Debug(this.name)
     this.version = this.pjson.version
diff --git a/src/config/ts-node.ts b/src/config/ts-node.ts
index 7a1f5ff..0f6f843 100644
--- a/src/config/ts-node.ts
+++ b/src/config/ts-node.ts
@@ -2,9 +2,11 @@ import * as fs from 'fs'
 import * as path from 'path'
 
 import {TSConfig} from '../interfaces/ts-config'
+import {settings} from '../settings'
+import {isProd} from '../util'
 import {Debug} from './util'
 // eslint-disable-next-line new-cap
-const debug = Debug()
+const debug = Debug('ts-node')
 
 function loadTSConfig(root: string): TSConfig | undefined {
   const tsconfigPath = path.join(root, 'tsconfig.json')
@@ -41,7 +43,15 @@ export function tsPath(root: string, orig: string | undefined): string | undefin
 export function tsPath(root: string, orig: string | undefined): string | undefined {
   if (!orig) return orig
   orig = path.join(root, orig)
-  if (process.env.NODE_ENV !== 'development') return orig
+
+  const skipTSNode =
+    // the CLI specifically turned it off
+    (settings.tsnodeEnabled === false) ||
+    // the CLI didn't specify ts-node and it is production
+    (settings.tsnodeEnabled === undefined && isProd())
+
+  if (skipTSNode) return orig
+
   try {
     const tsconfig = loadTSConfig(root)
     if (!tsconfig) return orig
@@ -52,7 +62,8 @@ export function tsPath(root: string, orig: string | undefined): string | undefin
     const lib = path.join(root, outDir) // ./lib
     const src = path.join(root, rootDirPath) // ./src
     const relative = path.relative(lib, orig) // ./commands
-    const out = path.join(src, relative) // ./src/commands
+    // For hooks, it might point to a js file, not a module. Something like "./hooks/myhook.js" which doesn't need the js.
+    const out = path.join(src, relative).replace(/\.js$/, '') // ./src/commands
     // this can be a directory of commands or point to a hook file
     // if it's a directory, we check if the path exists. If so, return the path to the directory.
     // For hooks, it might point to a module, not a file. Something like "./hooks/myhook"
diff --git a/src/config/util.ts b/src/config/util.ts
index 8c9befb..1b5fc5b 100644
--- a/src/config/util.ts
+++ b/src/config/util.ts
@@ -42,25 +42,6 @@ export function uniq<T>(arr: T[]): T[] {
   })
 }
 
-function termwidth(stream: any): number {
-  if (!stream.isTTY) {
-    return 80
-  }
-  const width = stream.getWindowSize()[0]
-  if (width < 1) {
-    return 80
-  }
-  if (width < 40) {
-    return 40
-  }
-  return width
-}
-
-const columns: number | null = (global as any).columns
-
-export const stdtermwidth = columns || termwidth(process.stdout)
-export const errtermwidth = columns || termwidth(process.stderr)
-
 function displayWarnings() {
   if (process.listenerCount('warning') > 1) return
   process.on('warning', (warning: any) => {
diff --git a/src/errors/config.ts b/src/errors/config.ts
index 331ed49..7a757f9 100644
--- a/src/errors/config.ts
+++ b/src/errors/config.ts
@@ -1,8 +1,6 @@
+import {settings} from '../settings'
 import {Logger} from './logger'
 
-// eslint-disable-next-line no-multi-assign
-const g = (global as any).oclif = (global as any).oclif || {}
-
 function displayWarnings() {
   if (process.listenerCount('warning') > 1) return
   process.on('warning', (warning: any) => {
@@ -14,18 +12,22 @@ function displayWarnings() {
 export const config = {
   errorLogger: undefined as Logger | undefined,
   get debug(): boolean {
-    return Boolean(g.debug)
+    return Boolean(settings.debug)
   },
   set debug(enabled: boolean) {
-    g.debug = enabled
+    settings.debug = enabled
     if (enabled) displayWarnings()
   },
   get errlog(): string | undefined {
-    return g.errlog
+    return settings.errlog
   },
   set errlog(errlog: string | undefined) {
-    g.errlog = errlog
-    if (errlog) this.errorLogger = new Logger(errlog)
-    else delete this.errorLogger
+    if (errlog) {
+      this.errorLogger = new Logger(errlog)
+      settings.errlog = errlog
+    } else {
+      delete this.errorLogger
+      delete settings.errlog
+    }
   },
 }
diff --git a/src/errors/errors/pretty-print.ts b/src/errors/errors/pretty-print.ts
index 379ead8..6b4a5b2 100644
--- a/src/errors/errors/pretty-print.ts
+++ b/src/errors/errors/pretty-print.ts
@@ -1,7 +1,7 @@
 import * as wrap from 'wrap-ansi'
 import indent = require('indent-string')
 
-import * as screen from '../screen'
+import * as screen from '../../screen'
 import {config} from '../config'
 import {PrettyPrintableError} from '../../interfaces/errors'
 
diff --git a/src/errors/global.d.ts b/src/errors/global.d.ts
deleted file mode 100644
index f020b22..0000000
--- a/src/errors/global.d.ts
+++ /dev/null
@@ -1,10 +0,0 @@
-// tslint:disable
-
-declare namespace NodeJS {
-  interface Global {
-    oclif?: {
-      debug?: boolean;
-      errlog?: string;
-    };
-  }
-}
diff --git a/src/errors/screen.ts b/src/errors/screen.ts
deleted file mode 100644
index 1b6e27c..0000000
--- a/src/errors/screen.ts
+++ /dev/null
@@ -1,18 +0,0 @@
-function termwidth(stream: any): number {
-  if (!stream.isTTY) {
-    return 80
-  }
-  const width = stream.getWindowSize()[0]
-  if (width < 1) {
-    return 80
-  }
-  if (width < 40) {
-    return 40
-  }
-  return width
-}
-
-const columns: number | null = (global as any).columns
-
-export const stdtermwidth = columns || termwidth(process.stdout)
-export const errtermwidth = columns || termwidth(process.stderr)
diff --git a/src/help/global.d.ts b/src/help/global.d.ts
deleted file mode 100644
index 9ea9756..0000000
--- a/src/help/global.d.ts
+++ /dev/null
@@ -1,5 +0,0 @@
-declare namespace NodeJS {
-  interface Global {
-    'columns': number;
-  }
-}
diff --git a/src/help/index.ts b/src/help/index.ts
index 359c6a0..37426b8 100644
--- a/src/help/index.ts
+++ b/src/help/index.ts
@@ -7,7 +7,7 @@ import {error} from '../errors'
 import CommandHelp from './command'
 import {renderList} from './list'
 import RootHelp from './root'
-import {stdtermwidth} from './screen'
+import {stdtermwidth} from '../screen'
 import {compact, sortBy, uniqBy} from '../util'
 import {standarizeIDFromArgv, template} from './util'
 
diff --git a/src/help/screen.ts b/src/help/screen.ts
deleted file mode 100644
index 9cb0e44..0000000
--- a/src/help/screen.ts
+++ /dev/null
@@ -1,18 +0,0 @@
-function termwidth(stream: any): number {
-  if (!stream.isTTY) {
-    return 80
-  }
-  const width = stream.getWindowSize()[0]
-  if (width < 1) {
-    return 80
-  }
-  if (width < 40) {
-    return 40
-  }
-  return width
-}
-
-const columns: number | null = parseInt(process.env.COLUMNS!, 10) || (global as any).columns
-
-export const stdtermwidth = columns || termwidth(process.stdout)
-export const errtermwidth = columns || termwidth(process.stderr)
diff --git a/src/index.ts b/src/index.ts
index 3fcedf9..806afe7 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -11,6 +11,7 @@ import {HelpBase, Help, getHelpClass} from './help'
 import {toStandardizedId, toConfiguredId} from './help/util'
 import * as Parser from './parser'
 import {Hook} from './interfaces/hooks'
+import {settings, Settings} from './settings'
 
 export {
   Command,
@@ -29,6 +30,8 @@ export {
   tsPath,
   toStandardizedId,
   toConfiguredId,
+  settings,
+  Settings,
 }
 
 function checkCWD() {
diff --git a/src/parser/list.ts b/src/parser/list.ts
index 2d31e91..2156f1b 100644
--- a/src/parser/list.ts
+++ b/src/parser/list.ts
@@ -1,4 +1,4 @@
-import {stdtermwidth} from './screen'
+import {stdtermwidth} from '../screen'
 import {maxBy} from './util'
 import {List} from '../interfaces'
 
@@ -13,7 +13,7 @@ export function renderList(items: List): string {
   if (items.length === 0) {
     return ''
   }
-  const maxLength = (maxBy(items, i => i[0].length))![0].length
+  const maxLength = maxBy(items, i => i[0].length)![0].length
   const lines = items.map(i => {
     let left = i[0]
     let right = i[1]
diff --git a/src/parser/screen.ts b/src/screen.ts
similarity index 75%
rename from src/parser/screen.ts
rename to src/screen.ts
index 1b6e27c..c188640 100644
--- a/src/parser/screen.ts
+++ b/src/screen.ts
@@ -1,3 +1,5 @@
+import {settings} from './settings'
+
 function termwidth(stream: any): number {
   if (!stream.isTTY) {
     return 80
@@ -12,7 +14,7 @@ function termwidth(stream: any): number {
   return width
 }
 
-const columns: number | null = (global as any).columns
+const columns = parseInt(process.env.OCLIF_COLUMNS!, 10) || settings.columns
 
 export const stdtermwidth = columns || termwidth(process.stdout)
 export const errtermwidth = columns || termwidth(process.stderr)
diff --git a/src/settings.ts b/src/settings.ts
new file mode 100644
index 0000000..8f098c1
--- /dev/null
+++ b/src/settings.ts
@@ -0,0 +1,38 @@
+export type Settings = {
+  /**
+   * Show additional debug output without DEBUG. Mainly shows stackstraces.
+   *
+   * Useful to set in the ./bin/dev script.
+   * oclif.settings.debug = true;
+   */
+  debug?: boolean;
+  /**
+   * The path to the error.log file.
+   *
+   * NOTE: This is read-only and setting it will have no effect.
+   */
+  errlog?: string;
+  /**
+   * Set the terminal width to a specified number of columns (characters)
+   *
+   * Environment Variable:
+   *   OCLIF_COLUMNS=80
+   */
+  columns?: number;
+  /**
+   * Try to use ts-node to load typescript source files instead of
+   * javascript files.
+   *
+   * NOTE: This requires registering ts-node first.
+   * require('ts-node').register();
+   *
+   * Environment Variable:
+   *   NODE_ENV=development
+   */
+  tsnodeEnabled?: boolean;
+};
+
+// Set global.oclif to the new object if it wasn't set before
+if (!(global as any).oclif) (global as any).oclif = {}
+
+export const settings: Settings = (global as any).oclif as Settings
diff --git a/src/util.ts b/src/util.ts
index 155c708..3d97d40 100644
--- a/src/util.ts
+++ b/src/util.ts
@@ -35,3 +35,7 @@ export function castArray<T>(input?: T | T[]): T[] {
   if (input === undefined) return []
   return Array.isArray(input) ? input : [input]
 }
+
+export function isProd() {
+  return process.env.NODE_ENV !== 'development'
+}


Test patch:
diff --git a/test/config/ts-node.test.ts b/test/config/ts-node.test.ts
index 7b7678b..895c6e7 100644
--- a/test/config/ts-node.test.ts
+++ b/test/config/ts-node.test.ts
@@ -2,16 +2,24 @@ import * as path from 'path'
 import * as proxyquire from 'proxyquire'
 import * as tsNode from 'ts-node'
 
-import {Interfaces} from '../../src'
+import {Interfaces, settings} from '../../src'
 
 import {expect, fancy} from './test'
 
 const root = path.resolve(__dirname, 'fixtures/typescript')
-const orig = 'src/hooks/init.ts'
+const tsSource = 'src/hooks/init.ts'
+// ts-node can load the file as a module (without ts)
+const tsModule = 'src/hooks/init'
+const jsCompiledModule = 'lib/hooks/init'
+const jsCompiled = 'lib/hooks/init.js'
 let tsNodeRegisterCallArguments: any[] = []
 
+// Typical root and out options of a typescript project
 const DEFAULT_TS_CONFIG: Interfaces.TSConfig = {
-  compilerOptions: {},
+  compilerOptions: {
+    rootDir: 'src',
+    outDir: 'lib',
+  },
 }
 
 const withMockTsConfig = (config: Interfaces.TSConfig = DEFAULT_TS_CONFIG) => {
@@ -34,9 +42,53 @@ const withMockTsConfig = (config: Interfaces.TSConfig = DEFAULT_TS_CONFIG) => {
 }
 
 describe('tsPath', () => {
+  withMockTsConfig()
+  .it('should resolve a .js file to ts src', (ctx: any) => {
+    const result = ctx.tsNodePlugin.tsPath(root, jsCompiled)
+    expect(result).to.equal(path.join(root, tsModule))
+  })
+
+  withMockTsConfig()
+  .it('should resolve a module file to ts src', (ctx: any) => {
+    const result = ctx.tsNodePlugin.tsPath(root, jsCompiledModule)
+    expect(result).to.equal(path.join(root, tsModule))
+  })
+
   withMockTsConfig()
   .it('should resolve a .ts file', (ctx: any) => {
-    const result = ctx.tsNodePlugin.tsPath(root, orig)
-    expect(result).to.equal(path.join(root, orig))
+    const result = ctx.tsNodePlugin.tsPath(root, tsSource)
+    expect(result).to.equal(path.join(root, tsSource))
+  })
+
+  withMockTsConfig({compilerOptions: {}})
+  .it('should resolve .js with no rootDir or outDir', (ctx: any) => {
+    const result = ctx.tsNodePlugin.tsPath(root, jsCompiled)
+    expect(result).to.equal(path.join(root, jsCompiled))
+  })
+
+  withMockTsConfig()
+  .do(() => {
+    settings.tsnodeEnabled = true
+    delete process.env.NODE_ENV
+  })
+  .finally(() => {
+    delete settings.tsnodeEnabled
+    process.env.NODE_ENV = 'development'
+  })
+  .it('should resolve to .ts file if enabled and prod', (ctx: any) => {
+    const result = ctx.tsNodePlugin.tsPath(root, jsCompiled)
+    expect(result).to.equal(path.join(root, tsModule))
+  })
+
+  withMockTsConfig()
+  .do(() => {
+    settings.tsnodeEnabled = false
+  })
+  .finally(() => {
+    delete settings.tsnodeEnabled
+  })
+  .it('should resolve to js if disabled', (ctx: any) => {
+    const result = ctx.tsNodePlugin.tsPath(root, jsCompiled)
+    expect(result).to.equal(path.join(root, jsCompiled))
   })
 })
diff --git a/test/help/format-command.test.ts b/test/help/format-command.test.ts
index 448be79..98febb6 100644
--- a/test/help/format-command.test.ts
+++ b/test/help/format-command.test.ts
@@ -5,7 +5,7 @@ import {Command as Base, Flags as flags, Interfaces, toCached} from '../../src'
 import {Help} from '../../src/help'
 
 const g: any = global
-g.columns = 80
+g.oclif.columns = 80
 
 class Command extends Base {
   async run() {
diff --git a/test/help/format-commands.test.ts b/test/help/format-commands.test.ts
index 6e0d62a..841e046 100644
--- a/test/help/format-commands.test.ts
+++ b/test/help/format-commands.test.ts
@@ -3,7 +3,7 @@ import {expect, test as base} from '@oclif/test'
 import stripAnsi = require('strip-ansi')
 
 const g: any = global
-g.columns = 80
+g.oclif.columns = 80
 import {Help} from '../../src/help'
 import {AppsDestroy, AppsCreate} from './fixtures/fixtures'
 
diff --git a/test/help/format-root.test.ts b/test/help/format-root.test.ts
index 35a63da..d9fb5c6 100644
--- a/test/help/format-root.test.ts
+++ b/test/help/format-root.test.ts
@@ -5,7 +5,7 @@ import {Help} from '../../src/help'
 import {Interfaces} from '../../src'
 
 const g: any = global
-g.columns = 80
+g.oclif.columns = 80
 
 const VERSION = require('../../package.json').version
 const UA = `@oclif/core/${VERSION} ${process.platform}-${process.arch} node-${process.version}`
diff --git a/test/help/format-topic.test.ts b/test/help/format-topic.test.ts
index 92d2c62..3149924 100644
--- a/test/help/format-topic.test.ts
+++ b/test/help/format-topic.test.ts
@@ -5,7 +5,7 @@ import {Help} from '../../src/help'
 import {Interfaces} from '../../src'
 
 const g: any = global
-g.columns = 80
+g.oclif.columns = 80
 
 // extensions to expose method as public for testing
 class TestHelp extends Help {
diff --git a/test/help/format-topics.test.ts b/test/help/format-topics.test.ts
index 73ee9de..4f17e39 100644
--- a/test/help/format-topics.test.ts
+++ b/test/help/format-topics.test.ts
@@ -5,7 +5,7 @@ import {Help} from '../../src/help'
 import {Interfaces} from '../../src'
 
 const g: any = global
-g.columns = 80
+g.oclif.columns = 80
 
 // extensions to expose method as public for testing
 class TestHelp extends Help {
diff --git a/test/help/show-help.test.ts b/test/help/show-help.test.ts
index ef6d4a6..011ce6c 100644
--- a/test/help/show-help.test.ts
+++ b/test/help/show-help.test.ts
@@ -7,7 +7,7 @@ import {AppsIndex, AppsDestroy, AppsCreate, AppsTopic, AppsAdminTopic, AppsAdmin
 import {Interfaces, Config} from '../../src'
 
 const g: any = global
-g.columns = 80
+g.oclif.columns = 80
 
 // extension makes previously protected methods public
 class TestHelp extends Help {
diff --git a/test/helpers/init.js b/test/helpers/init.js
index 7866e4b..338e715 100644
--- a/test/helpers/init.js
+++ b/test/helpers/init.js
@@ -2,4 +2,5 @@ const path = require('path')
 process.env.TS_NODE_PROJECT = path.resolve('test/tsconfig.json')
 process.env.NODE_ENV = 'development'
 
-global.columns = 80
+global.oclif = global.oclif || {}
+global.oclif.columns = 80