Skip to content

Provide document path context for input validation errors. #3189

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

Merged
merged 4 commits into from
Jun 24, 2020

Conversation

wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Jun 8, 2020

Potentially addresses #3169.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 8, 2020

Binary Size Report

Affected SDKs

  • @firebase/firestore

    Type Base (1791a9b) Head (4743499) Diff
    browser 248 kB 248 kB -12 B (-0.0%)
    esm2017 194 kB 194 kB -14 B (-0.0%)
    main 493 kB 493 kB +385 B (+0.1%)
    module 246 kB 246 kB -12 B (-0.0%)
  • @firebase/firestore/lite

    Type Base (1791a9b) Head (4743499) Diff
    main 140 kB 140 kB +389 B (+0.3%)
  • @firebase/firestore/memory

    Type Base (1791a9b) Head (4743499) Diff
    browser 186 kB 186 kB -12 B (-0.0%)
    esm2017 145 kB 145 kB -14 B (-0.0%)
    main 363 kB 363 kB +385 B (+0.1%)
    module 184 kB 184 kB -12 B (-0.0%)
  • firebase

    Type Base (1791a9b) Head (4743499) Diff
    firebase-firestore.js 287 kB 287 kB -16 B (-0.0%)
    firebase-firestore.memory.js 226 kB 226 kB -16 B (-0.0%)
    firebase.js 820 kB 820 kB -16 B (-0.0%)

Test Logs

@@ -107,11 +107,12 @@ export class ArrayUnionFieldValueImpl extends SerializableFieldValue {

_toFieldTransform(context: ParseContext): FieldTransform {
// Although array transforms are used with writes, the actual elements
// being uniomed or removed are not considered writes since they cannot
// being unioned or removed are not considered writes since they cannot
// contain any FieldValue sentinels, etc.
const parseContext = new ParseContext(
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be time to add a helper method to ParseContext (or a tree-shakeable function in this file) that simplifies the creation of an argument parser. If arrayElement remains configurable, this code would replace line 112-122, 145-155 and 173-181.

Optional.

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 pulled out createSentinelChildContext. LMK if this is what you had in mind.

description += ' (found';

if (hasPath) {
description += ` in field ${path!.toString()}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: ${path!.toString()} is the same as ${path} (here and below).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for the good explanation in the comment!

@wilhuff wilhuff merged commit a1081c6 into master Jun 24, 2020
@wilhuff wilhuff deleted the wilhuff/doc-path-in-errors branch June 24, 2020 19:33
schmidt-sebastian added a commit that referenced this pull request Jun 24, 2020
The Lite tests are not yet run in CI. This PR updates the tests after #3189 and #3264.
@firebase firebase locked and limited conversation to collaborators Jul 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants