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

Enable strict function types #1082

Merged
merged 6 commits into from
Aug 2, 2018
Merged

Enable strict function types #1082

merged 6 commits into from
Aug 2, 2018

Conversation

gsoltis
Copy link

@gsoltis gsoltis commented Aug 1, 2018

After making some errors that would've been caught via more strict type checking, I explored what would be required to enable stricter checking.

Let me know what you think.

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.

Nice! These changes look reasonable to me. Thanks for improving our type checking.

@@ -60,7 +60,7 @@ export enum TimerId {
*
* Supports cancellation (via cancel()) and early execution (via skipDelay()).
*/
class DelayedOperation<T> implements CancelablePromise<T> {
class DelayedOperation<T extends AnyJs | void> implements CancelablePromise<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

All of this AnyJs | void repetition makes me wonder if there's a useful abstraction to extract... but I'm not quite sure. Does an OptionalAnyJs make sense (and would it be useful anywhere besides here)? Or perhaps an OperationReturnValue type alias in this file?

I don't feel strongly... we just repeat it half a dozen times here, so I'm wondering... It's probably fine as-is though.

Copy link
Author

Choose a reason for hiding this comment

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

I'll see what I can do. An alias local to one file it shouldn't be a problem, I don't think. void appears to be a bit of a weird concept at the intersection of Typescript and Javascript. See microsoft/TypeScript#20006 (comment) (but maybe avoid reading entire comment chain if you value your time and sanity). The gist of it is the AnyJs definition is "any value" (you can assign undefined to something), whereas, I think, void is intended to mean "no value" (let x = void; does not work). Per linked comment, you can technically end up with a value of void , but you probably shouldn't.

I think modifying our top type, AnyJs, to their recommended top type, null | undefined | {} might work, but I haven't tried it yet because that seems like it could have far-reaching implications. FWIW, Typescript 3.0 introduces unknown that I think is what we would actually want in this case: https://blogs.msdn.microsoft.com/typescript/2018/07/30/announcing-typescript-3-0/#the-unknown-type

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh, yes unknown will be nice to have. Good find.

To be clear, only "see what you can do" if you feel like it. This is okay as-is too.

@mikelehen mikelehen assigned gsoltis and unassigned mikelehen Aug 2, 2018
@gsoltis gsoltis merged commit 22ed571 into master Aug 2, 2018
@gsoltis gsoltis deleted the gsoltis/be_strict branch August 2, 2018 18:35
hiranya911 added a commit that referenced this pull request Aug 3, 2018
hiranya911 added a commit that referenced this pull request Aug 3, 2018
* Revert "Rxfire build fix (#1085)"

This reverts commit fd41378.

* Revert "Enable strict function types (#1082)"

This reverts commit 22ed571.

* Revert "Fixes servicerworker/client state syncing (#1074)"

This reverts commit 2dab48d.

* Revert "Prep rxfire release (#1079)"

This reverts commit c74c3b9.
@firebase firebase locked and limited conversation to collaborators Oct 17, 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.

4 participants