Remove asserts

[schmidt-sebastian]

This PR removes all asserts from our minified builds. This results in 7-10% savings before GZIP (see below).

The non-minified integration tests continue to run against the build with asserts. The Node build is not minified/mangled and continues to contains the asserts (referred to as "main" build below).

This is based on prior work in master...mikelehen:mikelehen/size-remove-asserts

[google-oss-bot]

Binary Size Report

Affected SDKs

SDK	Type	Base (a789d74)	Head (81db00f)	Diff
@firebase/firestore/memory	browser	209974.00	198105.00	-11869.00 (-5.65%)
	module	208377.00	196503.00	-11874.00 (-5.70%)
	esm2017	167439.00	155605.00	-11834.00 (-7.07%)
	main	374458.00	374451.00	-7.00 (-0.00%)
@firebase/firestore	browser	266685.00	252362.00	-14323.00 (-5.37%)
	module	264671.00	250342.00	-14329.00 (-5.41%)
	esm2017	213317.00	199040.00	-14277.00 (-6.69%)
	main	486146.00	486139.00	-7.00 (-0.00%)
firebase	firebase.js	840365.00	826028.00	-14337.00 (-1.71%)
	firebase-firestore.memory.js	253080.00	241200.00	-11880.00 (-4.69%)
	firebase-firestore.js	308468.00	294131.00	-14337.00 (-4.65%)

Metric Unit: byte

Test Logs

• https://github.com/firebase/firebase-js-sdk/actions/runs/73154133
• https://github.com/firebase/firebase-js-sdk/actions/runs/73178392

[wilhuff]

This change makes me deeply uncomfortable because we have no idea how the SDK will behave with assertions removed. I suspect we'd get close to the same savings by removing the text of the assertion without removing the actual check. That would at least avoid the possibility of proceeding in an undefined state.

If you really want to wholesale remove assertions, I think we need to audit just what kind of data corruption could be possible without them and probably turn those into errors we just handle directly. For example, if watch gives us corrupt responses, we currently assert about that, but we should instead just drop the message/reset the stream. Absent work like that, this seems pretty irresponsible.

[schmidt-sebastian]

This is a branch with all messages removed: #2820 (for now this is just done via refactoring, rather than code transformation. We likely want to stick with code transformation to make testing and development easier).

What stands out though is that almost all of our asserts are for internal state (like calling start() before using instance methods, type checking and in general verifying that our own generated data is sane). There are very few asserts that center around the backend protocol (but there are some, e.g. we verify the number of returned documents from a BatchGetDocuments and the number of returned mutation responses).

The size savings for the uncompressed memory-only build are reduced by roughly 3% if we only strip out the messages. The IndexedDB build is 2% smaller with the asserts completely removed.
With gzip, the "asserts but no messages" build is 84571 KB / 72665 KB (IndexedDB/memory-only, while the "no assert" build is 82635 KB / 71080 KB (~2.2% savings).

Note that Google strips asserts in its TypeScript build if "recommended settings" are used:
https://cs.corp.google.com/piper///depot/google3/javascript/tools/jscompiler/builddefs/flags.bzl?l=150
-> https://cs.corp.google.com/piper///depot/google3/javascript/tools/jscompiler/builddefs/flags.bzl?rcl=303238226&g=0&l=69

With out current pipeline, we could also treat the memory-only build different from the IndexedDB-build and only strip asserts from the memory-only build, which would reduce the risk of corruption. We could also start differentiating between "debugAsserts" and "runtimeAsserts" and keep the few asserts that take into account external input. Or we could decided that 2.2% is not worth this effort and just strip out the messages....

[wilhuff]

I favor differentiating debug asserts from runtime asserts, though maybe we could call those hardAssert to match Android and iOS naming?

Secondarily I wonder if we should replace hard asserts with proper error handling. That is: if we get an invalid value from watch, drop the whole message and proceed after logging rather than crashing?

I think we could proceed with this PR with only the first part and leave the latter suggestion for later.

[schmidt-sebastian]

Updated the PR to remove the newly added "debugAsserts". With this, the savings are now 5-7% since we are not (yet) special-casing fail.

[wilhuff]

LGTM