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

Remove JS input validation for Storage #3967

Merged
merged 5 commits into from
Oct 20, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

Removes all input validation that can be caught by TypeScript transpiler.

@changeset-bot
Copy link

changeset-bot bot commented Oct 20, 2020

⚠️ No Changeset found

Latest commit: 8cd023c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 20, 2020

Binary Size Report

Affected SDKs

  • @firebase/database

    Type Base (a5768b0) Head (6dbbe61) Diff
    browser 269 kB 270 kB +1.04 kB (+0.4%)
    esm2017 237 kB 238 kB +841 B (+0.4%)
    main 271 kB 272 kB +1.04 kB (+0.4%)
    module 269 kB 270 kB +1.04 kB (+0.4%)
  • @firebase/firestore

    Type Base (a5768b0) Head (6dbbe61) Diff
    browser 247 kB 239 kB -7.61 kB (-3.1%)
    esm2017 198 kB 190 kB -7.59 kB (-3.8%)
    main 484 kB 471 kB -13.2 kB (-2.7%)
    module 247 kB 239 kB -7.61 kB (-3.1%)
    react-native 198 kB 190 kB -7.60 kB (-3.8%)
  • @firebase/firestore/exp

    Type Base (a5768b0) Head (6dbbe61) Diff
    browser 190 kB 188 kB -1.40 kB (-0.7%)
    main 478 kB 476 kB -2.51 kB (-0.5%)
    module 190 kB 188 kB -1.40 kB (-0.7%)
    react-native 190 kB 189 kB -1.40 kB (-0.7%)
  • @firebase/firestore/lite

    Type Base (a5768b0) Head (6dbbe61) Diff
    browser 63.5 kB 62.2 kB -1.33 kB (-2.1%)
    main 140 kB 137 kB -2.54 kB (-1.8%)
    module 63.5 kB 62.2 kB -1.33 kB (-2.1%)
    react-native 63.7 kB 62.4 kB -1.33 kB (-2.1%)
  • @firebase/firestore/memory

    Type Base (a5768b0) Head (6dbbe61) Diff
    browser 184 kB 177 kB -7.60 kB (-4.1%)
    esm2017 148 kB 140 kB -7.57 kB (-5.1%)
    main 356 kB 343 kB -13.2 kB (-3.7%)
    module 184 kB 177 kB -7.60 kB (-4.1%)
    react-native 148 kB 140 kB -7.57 kB (-5.1%)
  • @firebase/storage

    Type Base (a5768b0) Head (6dbbe61) Diff
    esm2017 55.0 kB 49.3 kB -5.71 kB (-10.4%)
    main 61.4 kB 55.7 kB -5.66 kB (-9.2%)
    module 61.1 kB 55.4 kB -5.66 kB (-9.3%)
  • firebase

    Type Base (a5768b0) Head (6dbbe61) Diff
    firebase-database.js 189 kB 190 kB +1.00 kB (+0.5%)
    firebase-firestore.js 286 kB 278 kB -7.69 kB (-2.7%)
    firebase-firestore.memory.js 225 kB 217 kB -7.69 kB (-3.4%)
    firebase-storage.js 39.6 kB 36.4 kB -3.20 kB (-8.1%)
    firebase.js 831 kB 821 kB -9.90 kB (-1.2%)

Test Logs

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 20, 2020

Size Analysis Report

Affected Products

No changes between base commit (a5768b0) and head commit (6dbbe61).

Test Logs

if (!type.isObject(p) || !p) {
throw 'Expected Metadata object.';
}
export function validateMetadata(p: { [key: string]: unknown }): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually got rid of this entirely in storage-exp because it's checking types of Metadata fields which should be caught by typescript check? Or did I make a mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't realize this was fully typed. Removed.

@@ -353,3 +338,23 @@ export class Reference {
}
}
}

export function validateNumber(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go in a separate file? type.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to keep the original file for just this one function, but types.ts is a good location. Moved.

@Feiyang1
Copy link
Member

wow, impressive 10% saving!

@schmidt-sebastian schmidt-sebastian merged commit b247ffa into master Oct 20, 2020
@google-oss-bot google-oss-bot mentioned this pull request Oct 22, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/no-validation-storage branch November 9, 2020 22:36
@firebase firebase locked and limited conversation to collaborators Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants