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
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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.

},
"devDependencies": {
"@compodoc/compodoc": "1.1.19",
Expand All @@ -42,6 +43,7 @@
"@types/pumpify": "^1.4.1",
"@types/sinon": "^10.0.0",
"@types/uglify-js": "^3.17.0",
"@types/yargs": "^17.0.24",
"c8": "^7.0.0",
"codecov": "^3.1.0",
"execa": "^5.0.0",
Expand Down
28 changes: 28 additions & 0 deletions test/fixtures/echo.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* Copyright 2023 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// Lots of comments we can delete in uglify
async function main() {
// Showcases auto-pagination functionality.

// Let's say we have an API call that returns results grouped into pages.
// It accepts 4 parameters (just like gRPC stub calls do):
return 'SUCCESS';
}

main().catch(console.error);
// More comments

13 changes: 13 additions & 0 deletions test/unit/minify.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,17 @@ describe('minify tool', () => {
assert.deepEqual(objectBefore, objectAfter);
assert.deepEqual(objectBefore, parsedObjectAfter);
});

it('minifies the proto js file', async () => {
const echoProtoJsFixture = path.join(fixturesDir, 'echo.js');
const echoProtoJs = path.join(testDir, 'echo.js');
await fsp.copyFile(echoProtoJsFixture, echoProtoJs);
const statBefore = await fsp.stat(echoProtoJs);
const resultBefore = require(echoProtoJs);
await minify.main({directory: testDir, minifyJs: true});
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);

assert(statBefore.size > statAfter.size);
assert.deepEqual(resultBefore, resultAfter);
});
});
71 changes: 46 additions & 25 deletions tools/minify.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,49 +19,70 @@
import {promises as fsp} from 'fs';
import * as path from 'path';
import * as uglify from 'uglify-js';
import * as yargs from 'yargs';

async function minifyFile(filename: string) {
interface CliArgs {
directory?: string;
minifyJs?: boolean;
}
async function minifyFile(filename: string, isJs: boolean) {
const content = (await fsp.readFile(filename)).toString();
const output = uglify.minify(content, {
expression: true,
compress: false, // we need to keep it valid JSON
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.

output = uglify.minify(content, {
expression: true,
compress: false, // we need to keep it valid JSON
output: {quote_keys: true},
});
} else {
output = uglify.minify(content, {
expression: false,
});
}
if (output.error) {
throw output.error;
}
await fsp.writeFile(filename, output.code);
}

export async function main(directory?: string) {
const buildDir = directory ?? path.join(process.cwd(), 'build', 'protos');
export async function main(args?: CliArgs | string | undefined) {
let buildDir;
if (typeof args === 'object' && args.directory) {
buildDir = args.directory;
} else if (typeof args === 'string') {
buildDir = args;
} else {
buildDir = path.join(process.cwd(), 'build', 'protos');
}
const files = await fsp.readdir(buildDir);
const jsonFiles = files.filter(file => file.match(/\.json$/));
for (const jsonFile of jsonFiles) {
console.log(`Minifying ${jsonFile}...`);
await minifyFile(path.join(buildDir, jsonFile));
await minifyFile(path.join(buildDir, jsonFile), false);
}
if (typeof args === 'object' && args?.minifyJs) {
const jsFiles = files.filter(file => file.match(/\.js$/));
for (const jsFile of jsFiles) {
console.log(`Minifying ${jsFile}...`);
await minifyFile(path.join(buildDir, jsFile), true);
}
}
console.log('Minified all proto JSON files successfully.');
}

function usage() {
console.log(`Usage: node ${process.argv[1]} [directory]`);
console.log(
'Minifies all JSON files in-place in the given directory (non-recursively).'
);
console.log(
'If no directory is given, minifies JSON files in ./build/protos.'
`Minified all proto ${
typeof args === 'object' && args?.minifyJs ? 'JS and JSON' : 'JSON'
} files successfully.`
);
}

if (require.main === module) {
// argv[0] is node.js binary, argv[1] is script path

if (process.argv[2] === '--help') {
usage();
// eslint-disable-next-line no-process-exit
process.exit(1);
let args;
if (process.argv.length === 3 && !process.argv[2].includes('--directory')) {
args = process.argv[2] as string;
} else {
args = yargs(process.argv.slice(2)).usage(
`Usage: node ${process.argv[1]} [--directory] [--minifyJs]\nMinifies all JSON files in-place in the given directory (non-recursively).\nIf no directory is given, minifies JSON files in ./build/protos.\nCan minify JS files as well using --minifyJS flag`
).argv as unknown as CliArgs;
}

main(process.argv[2]);
main(args);
}