Moves cli to TypeScript and uses Inquirer.js
#86
Conversation
- Moves the remaining JS files in cli/ to TypeScript - Replaces 'prompt' with 'Inquirer.js'
| @@ -14,9 +14,7 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| 'use strict'; | |||
There was a problem hiding this comment.
how come we're removing this?
There was a problem hiding this comment.
We have strict: true on tsconfig.json, so TypeScript will add 'use strict'to the output JS for us.
| @@ -18,11 +18,10 @@ import {promisify} from 'util'; | |||
| import * as fs from 'fs'; | |||
| import * as path from 'path'; | |||
| import * as os from 'os'; | |||
| import userPrompt = require('prompt'); | |||
| import * as inquirer from 'inquirer'; | |||
There was a problem hiding this comment.
Not essential, but I'd still prefer we be precise with our imports.
There was a problem hiding this comment.
I'm a bit divided in the case where a library has many related methods.
When we import with import * as inquirer from 'inquirer', then we get to use inquirer.prompt(...) in the code, which makes it clear where prompt is coming from.
When we import with import {prompt} from 'inquirer', the call in the code becomes just prompt, which is less obvious where it comes from, unless you scan the imports.
WDYT?
| @@ -21,6 +21,7 @@ import {template} from 'lodash'; | |||
| import {promisify} from 'util'; | |||
| import {TwaManifest} from './TwaManifest'; | |||
| import Jimp = require('jimp'); | |||
| import Log from './Log'; | |||
There was a problem hiding this comment.
TypeScript question - why do some of these have {} and some don't?
There was a problem hiding this comment.
On the Log, we do export default class Log. on TwaManifest, only export class TwaManifest.
| import {isWebUri} from 'valid-url'; | ||
|
|
||
| export function validatePassword(input: string): boolean { | ||
| return input.length > 0; |
There was a problem hiding this comment.
Sure we don't want something longer?
There was a problem hiding this comment.
I need to look at the requirements for keytool and replicate there. Doing on a following CL.
src/cli/inputHelpers.ts
Outdated
| } | ||
|
|
||
| export function notEmpty(input: string): boolean { | ||
| return input.length > 0; |
| import prompt = require('prompt'); | ||
| // TODO(andreban): Remove usage of any | ||
| export function get(schema: any): any; // eslint-disable-line @typescript-eslint/no-explicit-any | ||
| import Color = require('color'); |
There was a problem hiding this comment.
And again with the imports questions, why is this require?
There was a problem hiding this comment.
I don't fully understand the require one, but it's related to the underlying library being written in JavaScript. Using import * as Color from 'color', import {Color} from 'color' or import Color from 'color' reports the error below:
Module '"/Users/andreban/Projects/twa-bootstrap/node_modules/@types/color/index"' can only be default-imported using the 'esModuleInterop' flagts(1259)
index.d.ts(135, 1): This module is declared with using 'export =', and can only be used with a default import when using the 'esModuleInterop' flag.
|
|
||
| async function help() { | ||
| export async function help(): Promise<void> { | ||
| return; |
There was a problem hiding this comment.
For now... Need to add actual help messages (I'll do in another PR. Focusing this one on translating JS => TS)
| type: 'input', | ||
| message: 'Domain being opened in the TWA:', | ||
| default: twaManifest.host, | ||
| validate: notEmpty, |
There was a problem hiding this comment.
We could check this is a valid URL (future CL though).
There was a problem hiding this comment.
This one is not a full URL, just the host component (I agree we should validate)
There was a problem hiding this comment.
#31 is the one I created for those.
| type: 'input', | ||
| message: 'Name of the application:', | ||
| default: twaManifest.name, | ||
| validate: notEmpty, |
There was a problem hiding this comment.
Similarly, we could check that this and launcherName aren't too long.
There was a problem hiding this comment.
Another good one for #31 (Adding here so I get the mentions in the issue)
| }, { | ||
| name: 'themeColor', | ||
| type: 'input', | ||
| message: 'Color to be used for the status bar:', |
There was a problem hiding this comment.
It's maybe worth putting an example of the value format (is it #232323, just 232323, either, both?).
There was a problem hiding this comment.
The default value shows the correct format (#232323). Is this enough?
There was a problem hiding this comment.
Also, we accept a wide range of formats. Including blue, rgb(0, 0, 255), etc.
Closes #76
Closes #79