Page MenuHomePhabricator

[Flow202][keyserver][skip-ci] [5/x] Address type errors in keyserver/src/push/utils.js
ClosedPublic

Authored by ashoat on Nov 13 2023, 2:48 PM.
Tags
None
Referenced Files
F3374128: D9853.id33191.diff
Tue, Nov 26, 1:59 PM
Unknown Object (File)
Sat, Nov 23, 9:38 AM
Unknown Object (File)
Fri, Nov 22, 10:20 PM
Unknown Object (File)
Fri, Nov 22, 10:01 PM
Unknown Object (File)
Fri, Nov 22, 9:58 PM
Unknown Object (File)
Fri, Nov 22, 5:59 PM
Unknown Object (File)
Sun, Oct 27, 11:29 PM
Unknown Object (File)
Oct 18 2024, 12:17 AM
Subscribers

Details

Summary

There are some noteworthy changes here – some necessary to address Flow issues, some opinionated ones from me, and in some cases the new version of Flow identified actual type issues.

As such, putting the changes in this file up for review separately. I'll annotate the noteworthy changes inline.

NOTE: CI will fail on this diff. I considered the possibility of fixing Flow errors BEFORE upgrading Flow, but it wasn't possible... in some cases, the fixes to support the new version of Flow caused errors in the old version. I could have hidden these type errors with $FlowFixMe lines and then later revert those, but that seemed like too much busy work.

Depends on D9852

Test Plan

Confirm the Flow errors go away

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

keyserver/src/push/utils.js
70 ↗(On Diff #33167)

mergedResults.sent is never used. It doesn't appear to be used when it was introduced in D7815

99 ↗(On Diff #33167)

The additional of Error | here is explained below

130–134 ↗(On Diff #33167)

Rewriting this as a map let me skip annotating some types

144–146 ↗(On Diff #33167)

This is necessary because of the Error | change mentioned above, which is explained below

Since Flow no longer believes that error is necessarily a FirebaseError, I have to add all of these runtime checks to confirm error.errorInfo.errorCode is what I think it is

197 ↗(On Diff #33167)

This is the reason for Error |. The exception caught here can be returned in the errors array.

(The truth is that you can throw anything in JS, not just an Error. But throwing Error is a convention.)

269 ↗(On Diff #33167)

Flow wanted me to pull this out

352 ↗(On Diff #33167)

Not sure when or why this was necessary, but there's no reason it would be risky to return result directly – it's constructed right above on line 340 and not passed to anyplace else

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 13 2023, 2:59 PM
Harbormaster failed remote builds in B24069: Diff 33167!

Just use mixed instead of Error

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 13 2023, 3:49 PM
Harbormaster failed remote builds in B24092: Diff 33191!
keyserver/src/push/utils.js
70 ↗(On Diff #33191)

mergedResults.sent is never used. It doesn't appear to be used when it was introduced in D7815

99 ↗(On Diff #33191)

The addition of | mixed here is explained below

130–134 ↗(On Diff #33191)

Rewriting this as a map let me skip annotating some types

144–148 ↗(On Diff #33191)

This is necessary because of the | mixed change mentioned above, which is explained below

Since Flow no longer believes that error is necessarily a FirebaseError, I have to add all of these runtime checks to confirm error.errorInfo.errorCode is what I think it is

199 ↗(On Diff #33191)

This is the reason for | mixed. The exception caught here can be returned in the errors array.

271 ↗(On Diff #33191)

Flow wanted me to pull this out

340 ↗(On Diff #33191)

Not sure when or why this was necessary, but there's no reason it would be risky to return result directly – it's constructed right above on line 340 and not passed to anyplace else

marcin added inline comments.
keyserver/src/push/utils.js
96 ↗(On Diff #33191)

Nit: Mutable sounds more natural to me.

This revision is now accepted and ready to land.Nov 15 2023, 1:19 AM

Some suggestions in inline comments, but looks good to me either way

keyserver/src/push/utils.js
144–148 ↗(On Diff #33191)

We could type the error as something like {type: 'fcm', error: FirebaseError} | {type: 'other', error: mixed} and avoid this

160–172 ↗(On Diff #33191)

I think we could potentially avoid introducing WritableFCMPushResult (and equivalent structs for other push providers) with this change.

keyserver/src/push/utils.js
96 ↗(On Diff #33191)

I hear you... it's more natural to me too. I initially opted for "Writable" as a contrast to Flow's $ReadOnly, though – I think read/write matches better than read/mutable. At this point it would take a lot of time to rename all of the Writable* types I've created

144–148 ↗(On Diff #33191)

That's fair. I'll take a look at doing that (without changing the format of what we store in MariaDB)

160–172 ↗(On Diff #33191)

True, but there's no code duplication in the WritableFCMPushResult definition, and this lets us avoid recreating the object on every write

Address @michal's feedback to avoid excessive runtime checks

ashoat retitled this revision from [Flow202][keyserver] [5/x] Address type errors in keyserver/src/push/utils.js to [Flow202][keyserver][skip-ci] [5/x] Address type errors in keyserver/src/push/utils.js.Nov 19 2023, 5:05 PM