Page MenuHomePhabricator

[native] Deprecate and move `updateClientDBThreadStoreThreadInfos` and `createUpdateDBOpsForThreadStoreThreadInfos`
ClosedPublic

Authored by atul on Feb 13 2024, 2:06 PM.
Tags
None
Referenced Files
F3535577: D11057.id37042.diff
Wed, Dec 25, 4:06 PM
Unknown Object (File)
Tue, Dec 24, 7:47 AM
Unknown Object (File)
Tue, Dec 24, 7:47 AM
Unknown Object (File)
Tue, Dec 24, 7:47 AM
Unknown Object (File)
Tue, Dec 24, 7:47 AM
Unknown Object (File)
Tue, Dec 24, 7:47 AM
Unknown Object (File)
Tue, Dec 24, 7:47 AM
Unknown Object (File)
Mon, Dec 23, 1:09 PM
Subscribers

Details

Summary

createUpdateDBOpsForThreadStoreThreadInfos consumes deprecatedConvertClientDBThreadInfoToRawThreadInfo, but based on the name createUpdateDBOpsForThreadStoreThreadInfos it's not clear that the function is only for handling "Legacy" RawThreadInfos. I should've made this more clear when I was doing the refactoring work.

We could modify these functions to handle both (previously known as) LegacyRawThreadInfos and MinimallyEncodedRawThreadInfos, but that would complicate the logic and make things tricky with flow types because we want to support Legacy -> Legacy, Legacy -> MinimallyEncoded, and MinimallyEncoded -> MinimallyEncoded migration functions. Whereas in the future we only care about MinimallyEncoded -> MinimallyEncoded migrations and supporting the legacy stuff just adds clutter.

It would also be good to keep these existing utilities "frozen" so we don't have to go back and test old migrations pre-minimalEncoding... which would be tedious and time consuming.

Personally think it makes sense to keep these utilities "frozen" in a deprecated-* file and introduce new functions that only handle MinimallyEncoded->MinimallyEncoded going forward.

This is kind of an opinionated decision, so adding @ashoat as reviewer to get his thoughts.


Depends on D11056

Test Plan

Simple rename and move, trusting flow.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul added inline comments.
native/redux/client-db-utils.js
12–14 ↗(On Diff #37042)

We have the import reordering again from Refactor -> Move tool, will update this diff.

atul requested review of this revision.Feb 13 2024, 2:21 PM
atul planned changes to this revision.Feb 13 2024, 2:43 PM
atul edited the summary of this revision. (Show Details)

Thanks for the explanation in the diff description. Your reasoning makes sense to me

This revision is now accepted and ready to land.Feb 15 2024, 12:29 PM
This revision was landed with ongoing or failed builds.Feb 15 2024, 1:42 PM
This revision was automatically updated to reflect the committed changes.