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: Add overloads for doc() and doc(id:string) #662

Merged
merged 2 commits into from
May 28, 2019

Conversation

schmidt-sebastian
Copy link
Contributor

This addresses the same issue as #658, but instead of relaxing our CollectionReference.doc() API, it fixes the types to reflect the invariant that we enforce via our argument validation. As in older releases, we still prevent users from passing "undefined", but with this PR this should generate a compile time error.

This is based on the following little experiment:

interface Foo {
    bar(): void;
    bar(baz: string): void;
}
class Foobar implements Foo {
    bar(baz?: string) {}
}
function createFoo(): Foo {
    return new Foobar();
}
const f = createFoo();
f.bar();
f.bar("foobar");
f.bar(undefined); // Compile error

Note that I am marking this as a "fix" rather than as a "breaking change" as any user that tried to use "undefined" before would have been broken already (and gotten a validation error). This PR will therefore go out with the next release, which is a minor version bump.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 28, 2019
@@ -2055,6 +2055,8 @@ export class CollectionReference extends Query {
});
}

doc(): DocumentReference;
doc(documentPath: string): DocumentReference;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This weird arrangement here is so that our JSDoc generation still works. The TypeScript compiler still validates that we do not pass "undefined" (hence the test change).

@codecov
Copy link

codecov bot commented May 28, 2019

Codecov Report

Merging #662 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #662      +/-   ##
==========================================
- Coverage   61.55%   61.46%   -0.09%     
==========================================
  Files          21       21              
  Lines        3418     3418              
  Branches      459      459              
==========================================
- Hits         2104     2101       -3     
- Misses       1252     1254       +2     
- Partials       62       63       +1
Impacted Files Coverage Δ
dev/src/reference.ts 99.01% <ø> (ø) ⬆️
dev/src/watch.ts 96.52% <0%> (-1.16%) ⬇️

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 eaf5a4e...da56b06. Read the comment docs.

@codecov
Copy link

codecov bot commented May 28, 2019

Codecov Report

Merging #662 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #662   +/-   ##
=======================================
  Coverage   61.54%   61.54%           
=======================================
  Files          21       21           
  Lines        3425     3425           
  Branches      460      460           
=======================================
  Hits         2108     2108           
  Misses       1254     1254           
  Partials       63       63
Impacted Files Coverage Δ
dev/src/reference.ts 99.01% <ø> (ø) ⬆️

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 56f5a17...901149a. Read the comment docs.

Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@schmidt-sebastian schmidt-sebastian merged commit cb189e9 into master May 28, 2019
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/doc branch May 30, 2019 17:53
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.

None yet

3 participants