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

Mikelehen/doc changes for each #825

Merged
merged 4 commits into from
May 16, 2018
Merged

Conversation

mikelehen
Copy link
Contributor

Add 'forEach' (and 'map' for good measure) to Array methods we intercept on QuerySnapshot.docChanges.

Would have prevented #824.

];
docChangesPropertiesToOverride.forEach(property => {
/**
* This will technically overwrite the `Function.prototype.length` property of
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update the comment to indicate that "this" indicates "overwriting length".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I rewrote the comment to hopefully be clearer:

  /**
   * We are (re-)defining properties on QuerySnapshot.prototype.docChanges which
   * is a Function. This could fail, in particular in the case of 'length' which
   * already exists on Function.prototype and on IE11 is improperly defined with
   * `{ configurable: false }`. So we wrap this in a try/catch to ensure that we
   * still have a functional SDK.
   */

I'll hold off on trying to submit this until you make the publishToNpm() style thing go away.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I like it a lot!

@mikelehen mikelehen merged commit fbd5bd6 into master May 16, 2018
@mikelehen mikelehen deleted the mikelehen/docChanges-forEach branch August 3, 2018 17:18
@firebase firebase locked and limited conversation to collaborators Oct 20, 2019
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.

3 participants