Skip to content
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

feat(tools): minify proto JS files #1435

Merged
merged 10 commits into from
Mar 30, 2023
Merged

feat(tools): minify proto JS files #1435

merged 10 commits into from
Mar 30, 2023

Conversation

sofisl
Copy link
Contributor

@sofisl sofisl commented Mar 28, 2023

This feature will allow us to minify JS files as described in the Reducing Nodejs Package Size doc

@sofisl sofisl requested review from a team as code owners March 28, 2023 23:29
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Mar 28, 2023
package.json Outdated
@@ -29,7 +29,8 @@
"proto3-json-serializer": "^1.0.0",
"protobufjs": "7.2.2",
"protobufjs-cli": "1.1.1",
"retry-request": "^5.0.0"
"retry-request": "^5.0.0",
"yargs": "^17.7.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@alexander-fenster alexander-fenster changed the title feat: add minifyJs option for minifyTools feat(tools): minify proto JS files Mar 29, 2023
const resultBefore = require(echoProtoJs);
await minify.main(testDir);
const statAfter = await fsp.stat(echoProtoJs);
const resultAfter = require(echoProtoJs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you also want to change this in the test above? I just copied your pattern for the previous test:

const objectBefore = require(echoProtoJson);

@sofisl sofisl merged commit 014fa16 into main Mar 30, 2023
@sofisl sofisl deleted the optionallyMinifyJsFiles branch March 30, 2023 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants