Conversation
package.json
Outdated
| "protobufjs-cli": "1.1.1", | ||
| "retry-request": "^5.0.0" | ||
| "retry-request": "^5.0.0", | ||
| "yargs": "^17.7.1" |
There was a problem hiding this comment.
I'm looking forward to the split of google-gax into two modules :) For now, I'm hesitant of adding yargs as a runtime dependency. Even though it's not used by the code in runtime, it still adds a whole megabyte (!!!) to node_modules:
fenster-macbookpro:y fenster$ cat package.json
{
"dependencies": {
"yargs": "^17.7.1"
}
}
fenster-macbookpro:y fenster$ npm install
added 16 packages, and audited 17 packages in 226ms
2 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
fenster-macbookpro:y fenster$ du -sh node_modules/
1.0M node_modules/
which will increase not only the download size, but also a chance of us getting security vulnerability bugs. Since it's just about one extra command line option, would it be possible to parse argv manually? A simple for loop over argv should be enough here.
tools/minify.ts
Outdated
| output: {quote_keys: true}, | ||
| }); | ||
| let output; | ||
| if (!isJs) { |
There was a problem hiding this comment.
style nit: it will be a little bit more readable if we do
let options;
if (isJs) {
options = { ... };
} else {
options = { ... };
}
const output = uglify.minify(content, options);so we call minify just once in the code.
| const resultBefore = require(echoProtoJs); | ||
| await minify.main(testDir); | ||
| const statAfter = await fsp.stat(echoProtoJs); | ||
| const resultAfter = require(echoProtoJs); |
There was a problem hiding this comment.
Here I'm wondering if the second require of the same filename will just take it from cache. Maybe either invalidate the cache (https://stackoverflow.com/questions/9210542/node-js-require-cache-possible-to-invalidate) or just copy it into another file just to be sure.
There was a problem hiding this comment.
Would you also want to change this in the test above? I just copied your pattern for the previous test:
gax-nodejs/test/unit/minify.ts
Line 41 in 4d59f35
This feature will allow us to minify JS files as described in the
Reducing Nodejs Package Sizedoc