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

fix: support Query.stream() as first client operation #971

Merged
merged 3 commits into from
Mar 16, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

This fixes a regression introduced in 3.7

Fixes #969

This fixes a regression introduced in 3.7
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 16, 2020
@codecov
Copy link

codecov bot commented Mar 16, 2020

Codecov Report

Merging #971 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #971      +/-   ##
==========================================
- Coverage   97.55%   97.45%   -0.11%     
==========================================
  Files          25       25              
  Lines       15983    15978       -5     
  Branches     1283     1208      -75     
==========================================
- Hits        15593    15572      -21     
- Misses        387      403      +16     
  Partials        3        3
Impacted Files Coverage Δ
dev/src/reference.ts 99.8% <100%> (-0.08%) ⬇️
dev/src/order.ts 98.09% <0%> (-1.15%) ⬇️
dev/src/pool.ts 97.75% <0%> (-0.9%) ⬇️
dev/src/transaction.ts 96.47% <0%> (-0.51%) ⬇️
dev/src/watch.ts 98.77% <0%> (-0.37%) ⬇️
dev/src/serializer.ts 98.77% <0%> (-0.25%) ⬇️
dev/src/validate.ts 98.05% <0%> (-0.25%) ⬇️
dev/src/index.ts 98.62% <0%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 469df02...7cf758d. Read the comment docs.

@@ -1277,7 +1280,8 @@ describe('limitToLast() interface', () => {

it('requires at least one ordering constraints', () => {
const query = firestore.collection('collectionId');
expect(() => query.limitToLast(1).get()).to.throw(
const result = query.limitToLast(1).get();
return expect(result).to.eventually.be.rejectedWith(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the refactor in the original PR was meant to ensure that this is thrown as an exception, rather than as a rejected Promise, but it is probably not worth breaking the client for.

@@ -1989,11 +1986,11 @@ export class Query<T = DocumentData> {
/**
* Internal streaming method that accepts the request proto.
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is now outdated.

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

);
this.firestore
.initializeIfNeeded(tag)
.then(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

async is not needed, there is no await inside.

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 added a comment. async makes sure that any exception from this.toProto is converted into a rejected Promise.

@schmidt-sebastian schmidt-sebastian merged commit a48017c into master Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrading from 3.6.0 to 3.7.0 throws internal error.
3 participants