Remove JS Input Validation #3939
Conversation
🦋 Changeset detectedLatest commit: 60d46aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
schmidt-sebastian
force-pushed
the
mrschmidt/no-validation
branch
from
1038763
to
ff78c48
Compare
Binary Size ReportAffected SDKs
Test Logs
|
Size Analysis ReportAffected ProductsNo changes between base commit (0204abf) and head commit (f6ff48c). Test Logs
|
schmidt-sebastian
force-pushed
the
mrschmidt/no-validation
branch
from
2ea6f7d
to
b84523f
Compare
|
I sat on this for too long since I was hoping this would shave off even more bytes, but I was just informed that code freeze for Fireconf is tomorrow. |
| @@ -189,6 +190,7 @@ export class Transaction | |||
| options?: legacy.SetOptions | |||
| ): Transaction { | |||
| if (options) { | |||
| validateSetOptions('Transaction.set', options); | |||
There was a problem hiding this comment.
We still need to validate that merge and mergeFields isn't used together.
| @@ -69,7 +68,6 @@ export function serverTimestamp(): FieldValue { | |||
| * `updateDoc()`. | |||
| */ | |||
| export function arrayUnion(...elements: unknown[]): FieldValue { | |||
| validateAtLeastNumberOfArgs('arrayUnion()', arguments, 1); | |||
There was a problem hiding this comment.
While this make a little bit of sense, it is absent on Android.
| 'boolean', | ||
| 'experimentalAutoDetectLongPolling', | ||
| settings.experimentalAutoDetectLongPolling | ||
| ); | ||
| this.experimentalAutoDetectLongPolling = | ||
| settings.experimentalAutoDetectLongPolling ?? |
There was a problem hiding this comment.
So given that we're not validating that settings.experimentalAutoDetectLongPolling is actually a boolean value, should we be accessing it with a forced coercion (i.e. as !!settings.experimentalAutoDetectLongPolling)?
Or is the idea that we've declared the type in typescript and all bets are off for users who aren't using our d.ts files?
There was a problem hiding this comment.
I originally decided against this approach, because it makes it impossible to turn a feature off it its default value is true. Since we have only ever been using opt-in features, this has not been a problem yet. I updated this code, which ensures that we have a boolean value once we exit this function (it also removes some code).
| @@ -1478,10 +1346,8 @@ export class DocumentSnapshot<T = DocumentData> | |||
|
|
|||
| get( | |||
| fieldPath: string | ExternalFieldPath, | |||
| options?: PublicSnapshotOptions | |||
| options: PublicSnapshotOptions = {} | |||
There was a problem hiding this comment.
Does this require an API change to the d.ts file, or is this rendered by tsc as a local assignment if not defined? Stated alternatively: does the caller supply the default argument or does the callee?
There was a problem hiding this comment.
The default argument is provided by the callee. An example in ES3:
function foo(bar = {}) {
}
transpiles to:
"use strict";
function foo(bar) {
if (bar === void 0) { bar = {}; }
}
ES5+JavaScript supports default arguments, but the semantics match the code snippet above.
| @@ -2487,7 +2274,7 @@ export class CollectionReference<T = DocumentData> | |||
| } | |||
| } | |||
|
|
|||
| function validateSetOptions( | |||
| export function validateSetOptions( | |||
There was a problem hiding this comment.
Doesn't this potentially create a cycle? Better to move this into input_validation.ts.
This PR removes all input validation that should be caught by any JavaScript environments that supports strict type checking, such as TypeScript or Google Closure. It is merely meant to reduce binary size.
Validations that remain:
firestore-expAPI still callscastto verify that the passed in objects are the right Firestore objects. This should catch almost all cases where a user forgets to change the arguments as they transition to the functional API.