Fix: reduce huge bundle size by adding ADVANCED_OPTIMIZATIONS when compiling protobuf JS

[sayjeyhi]

This will close: #124

[sayjeyhi]

This will also fixe #92

After optimization, the bundle size went from ~156KB to ~47KB (uncompressed)

[dibenede]

Thanks for porting this over!

It looks like JSC is unhappy with @export annotations on functions that already have other visibility modifiers. If I'm aligning the errors correctly, some of the offending functions are currently marked as @Protected (e.g. Message.getRepeatedFloatingPointField). Is @export really required for these?

[sayjeyhi]

It looks like JSC is unhappy with @export annotations on functions that already have other visibility modifiers. If I'm aligning the errors correctly, some of the offending functions are currently marked as @Protected (e.g. Message.getRepeatedFloatingPointField). Is @export really required for these?

Yes, since we are removing them from deps, we need to tell JSC to bring the required ones by putting @export annotation there, to allow them to be bundled and used, those duplicate visibility modifier errors were only warnings, but I've fixed them on my last commit.
Still 2 tests are failing there:

Failures:
1) protoBinaryTest testUnknownExtension
  Message:
    TypeError: jspb.BinaryWriter is not a constructor
  Stack:
        at <Jasmine>
        at UserContext.<anonymous> (/Users/sayjeyhi/Desktop/Projects/github/protobuf-javascript/commonjs_out/binary/proto_test.js:643:18)
        at <Jasmine>
        at processImmediate (internal/timers.js:464:21)

2) Message test suite testCopyInto_notSameType
  Message:
    TypeError: Cannot read property 'error' of undefined
  Stack:
        at <Jasmine>
        at Object.goog.testing.asserts.raiseException (/Users/sayjeyhi/Desktop/Projects/github/protobuf-javascript/commonjs_out/test_node_modules/closure_asserts_commonjs.js:136:401)
        at goog.testing.asserts.assertThrows (/Users/sayjeyhi/Desktop/Projects/github/protobuf-javascript/commonjs_out/test_node_modules/closure_asserts_commonjs.js:97:24)
        at UserContext.<anonymous> (/Users/sayjeyhi/Desktop/Projects/github/protobuf-javascript/commonjs_out/message_test.js:567:13)
        at <Jasmine>
        at processImmediate (internal/timers.js:464:21)

87 specs, 2 failures
Finished in 0.081 seconds
Randomized with seed 81572 (jasmine --random=true --seed=81572)

Do you have any idea on how we can fix them?

[sayjeyhi]

I fixed one of the tests, but the other one is still failing:

Failures:
1) Message test suite testCopyInto_notSameType
  Message:
    TypeError: Cannot read property 'error' of undefined
  Stack:
        at <Jasmine>
        at Object.goog.testing.asserts.raiseException (/Users/sayjeyhi/Desktop/Projects/github/protobuf-javascript/commonjs_out/test_node_modules/closure_asserts_commonjs.js:136:401)
        at goog.testing.asserts.assertThrows (/Users/sayjeyhi/Desktop/Projects/github/protobuf-javascript/commonjs_out/test_node_modules/closure_asserts_commonjs.js:97:24)
        at UserContext.<anonymous> (/Users/sayjeyhi/Desktop/Projects/github/protobuf-javascript/commonjs_out/message_test.js:567:13)
        at <Jasmine>
        at processImmediate (internal/timers.js:464:21)

87 specs, 1 failure
Finished in 0.078 seconds
Randomized with seed 58126 (jasmine --random=true --seed=58126)


[17:40:50] 'test_commonjs' errored after 213 ms
[17:40:50] Error: Command failed: sh -c cd commonjs_out && JASMINE_CONFIG_PATH=jasmine.json NODE_PATH=test_node_modules ../node_modules/.bin/jasmine

[dibenede]

It looks like it's not finding goog.global.console.error in commonjs testing mode. It looks like the closure output mode tests do pass though..

Digging around, I see that we have a rewriting script to produce a new version message_test.js for the purposes of testing commonjs. I'm suspicious of that script and how it may be interacting with tree shaking:

protobuf-javascript/commonjs/rewrite_tests_for_commonjs.js

Line 69 in a428c58

console.log("global.goog = testdeps.goog;");

[sayjeyhi]

@dibenede, Thanks for the review, Yeah I've checked them and even tried to require the assertion library, but it seems
The tests were passing without a reliable reason and broke with our unwanted libs removal.

cuz only this test is failing, because assertThrows used in this test:

  it('testCopyInto_notSameType', function() {
    var a = new proto.jspb.test.TestClone();
    var b = new proto.jspb.test.Simple1(['str', ['s1', 's2']]);

    var e = assertThrows(function() {
      jspb.Message.copyInto(a, b);
    });
    assertContains('should have the same type', e.message);
  });

and assertThrows is using raiseException internally:

goog.testing.asserts.raiseException = function(comment, opt_message) {
  var e = new goog.testing.JsUnitException(comment, opt_message);

  var testCase = _getCurrentTestCase();
  if (testCase) {
    testCase.raiseAssertionException(e);
  } else {
    goog.global.console.error(
        'Failed to save thrown exception: no test case is installed.');
    throw e;
  }
};

when this _getCurrentTestCase couldn't get the test case else to block fires, and goog.global.console is used without checking if it is imported and it is giving us an error.

IDK, maybe we can ignore this test, or we can rewrite it using other test functions rather than assertThrows

[dibenede]

I attempted rewriting as well as deleting it, but there are other errors that crop up:

1) Message test suite testExtendedMessageEnsureObject
  Message:
    TypeError: Cannot read properties of undefined (reading 'a_key')
  Stack:
        at <Jasmine>
        at UserContext.<anonymous> (protobuf-javascript/commonjs_out/message_test.js:742:55)
        at <Jasmine>
        at listOnTimeout (node:internal/timers:564:17)
        at process.processTimers (node:internal/timers:507:7)

Based off the rewrite attempt, I think it's being set off by any assert* usage :/

[sayjeyhi]

First, I removed that test, and then the tests ran fine!

Then, I fixed the test to throw an exception, and then the test is running fine now without removing it.

[sayjeyhi]

@dibenede, I fixed these comments from @lukesandberg as well, and the tests are running correctly

[sayjeyhi]

@dibenede, thanks for merging this. when will there be a new package release? so we can test this out.

[dibenede]

I'm actively working on #136, which is a blocker for release. I'm hoping we can cut a release next week.