-
Notifications
You must be signed in to change notification settings - Fork 2
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
Assorted changes to webtreemap CLI tool #8
Conversation
f35dcf4
to
0fc6129
Compare
0fc6129
to
0260189
Compare
}, | ||
"devDependencies": { | ||
"@types/commander": "^2.11.0", | ||
"@types/node": "^8.0.34", | ||
"@types/node": "12.20.20", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This update includes the promises
bit of the fs
module. (I believe this was added in Node 10.)
@@ -76,14 +92,6 @@ function treeFromLines(lines: string[]): tree.Node { | |||
return node; | |||
} | |||
|
|||
function plainCaption(n: tree.Node): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was dead code
|
||
/** Write contents (utf-8 encoded) to a temp file, returning the path to the file. */ | ||
async function writeToTempFile(contents: string): Promise<string> { | ||
const randHex = crypto.randomBytes(4).readUInt32LE(0).toString(16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original PR used the tmp
module for this, but Evan was concerned about adding a new third party dep due to google3 rules. If that's no longer a concern, I'd certainly prefer to use the module.
@@ -34,28 +38,40 @@ async function readLines() { | |||
}); | |||
} | |||
|
|||
/** Reads a file to a string. */ | |||
async function readFile(path: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as the built-in fs.readFile
.
@@ -19,6 +19,7 @@ | |||
// "importHelpers": true, /* Import emit helpers from 'tslib'. */ | |||
// "downlevelIteration": true, /* Provide full support for iterables in 'for-of', spread, and destructuring when targeting 'ES5' or 'ES3'. */ | |||
// "isolatedModules": true, /* Transpile each file as a separate module (similar to 'ts.transpileModule'). */ | |||
"useDefineForClassFields": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be required to make rollup happy with TypeScript's JS output. See https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier
"compilerOptions": { | ||
/* Basic Options */ | ||
"target": "es2015", | ||
"module": "commonjs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This produces a CommonJS version of webtreemap (i.e. require
instead of import
) which can be run using node
.
(I'd be happy to debug the Vercel Deployment failure but I get a 404 when I click the Details link 🤷♂️) |
I wound up creating my own fork, these changes are available in the webtreemap-cli npm package: https://www.npmjs.com/package/webtreemap-cli Feel free to use anything in this PR if you like, though. |
This is copied over from evmar#35 which Evan isn't able to actively maintain (see evmar#37). I was thinking of forking evmar/webtreemap myself but then saw that @paulirish already had, so I figured I'd send this your way.
This expands the CLI tool to be a bit more usable:
I also updated the TypeScript and
@types/node
versions to be a bit more recent. I believe the CLI is broken in the current version (node doesn't likeimport
statements in a JS file). I believe I've fixed that.Let me know if this sort of change is welcome. If not I'm happy to go back to my own fork!