Page MenuHomePhabricator

[native] add entity and methods for redux-persist storage
ClosedPublic

Authored by kamil on Jul 11 2023, 4:19 AM.
Tags
None
Referenced Files
F2157199: D8472.id28932.diff
Mon, Jul 1, 1:23 AM
F2157198: D8472.id28585.diff
Mon, Jul 1, 1:23 AM
F2157177: D8472.diff
Mon, Jul 1, 1:19 AM
Unknown Object (File)
Mon, Jun 24, 11:41 PM
Unknown Object (File)
Mon, Jun 24, 12:06 AM
Unknown Object (File)
Sat, Jun 22, 5:56 AM
Unknown Object (File)
Fri, Jun 21, 10:55 PM
Unknown Object (File)
May 26 2024, 5:20 PM
Subscribers

Details

Summary

Mapping web methods: storage-engine-queries

Depends on D8471

Test Plan

Tests from D8557

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Jul 12 2023, 6:21 AM
michal requested changes to this revision.Jul 14 2023, 2:12 AM

Looks good, just some questions:

  • would it make sense to make these methods more inline with others? So e.g. setPersistStorageItem getting PersistItem as an argument, or getPersistStorageItem returning PersistItem or null instead of string?
  • can we test these methods, and amend the test plan?
This revision now requires changes to proceed.Jul 14 2023, 2:12 AM
kamil requested review of this revision.Jul 20 2023, 7:07 AM

Looks good, just some questions:

  • would it make sense to make these methods more inline with others? So e.g. setPersistStorageItem getting PersistItem as an argument, or getPersistStorageItem returning PersistItem or null instead of string?

That's an alternative approach, but I would prefer to stick to the current one:

  • this is key-value table, I would prefer to match other key-value APIs (see metadata)
  • this should replace the previous storage engine, eg. localstorage, and those functions match that API better
  • when we try to retrieve item at given key, I do not see a reason to return the object with key
  • can we test these methods, and amend the test plan?

Yes! I tested this in D8557 where I change tests to use new API

This revision is now accepted and ready to land.Jul 21 2023, 4:36 AM