File handling POC #102

kidroca · 2021-08-30T13:07:14Z

Details

Add file handling capabilities to Onyx

Uses localforage for web/desktop.

Extracted a web/desktop storage provider that saves all our key/value pairs in IndexedDB through localforage
This allows saving files as part of Onyx items

FIle saving on native

When an attachment is picked the AttahcmentPicker will persist it to App/cache folder. It is then available until cache is cleared
This lets us save only the reference to the File in our key/val storage
This reference is enough to work with the file - use it in an <Image /> or in a upload request

Related Issues

$ Expensify/App#3867

Automated Tests

TBD

Linked PRs

Captured existing storage interface from Onyx to separate file Storage provider mocking When we have provider.js and provider.native.js we need to have the mock implemented at the provider level instead of the AsyncStorage level

this should allow us to store and retrieve Files as well as anything else from storage for web and desktop

This is the same Provider we had up until now but the JSON parsing and stringifying is capsulated here instead of Onyx

This is handled by Storage providers

Use FileSystem from unimodules as that's how to use expo's file system on a bare RN project Ad helper methods to check file items

Instead of trying to guess and handle files leave this to happen explicitly by the library user

Add documentation and expose through Onyx

iOS would not create the folder automatically on copy

kidroca

Add some notes on the code to help review

lib/Onyx.js

lib/storage/index.native.js

kidroca

Pushing an update regarding the requested changes

It's seems this PR/POC is going to be accepted so I can add some tests tomorrow?

kidroca · 2021-11-03T20:53:03Z

tests/unit/cacheEvictionTest.js

@@ -1,4 +1,4 @@
-import AsyncStorage from '@react-native-async-storage/async-storage';
+import Storage from '../../lib/storage';


When we're testing Onyx specific logic like cache eviction we can use any mock that implements the methods of the StroageProvider interface - AsyncStorage's mock is serving ok here

The cache eviction logic cleans cache and relays updates to the storage provider - what the provider does with that information is up to the provider (e.g. the logic that is performed on setItem failure is performed regardless of the provider)

should we write tests that show the providers are equivalent?

We should write tests that show the providers do what they are intended to do

For AsyncStorage all that we do is JSON.stringify and JSON.parse the values we should have tests that we're passing the correct key and stringified value to AsyncStorage and there's a few exception cases where we shouldn't stringify null for example
We don't alter how multiSet and multiMerge work internally so we don't need to test the result - we'll just be testing how AsyncStorage works (also there's nothing immediate that we can do if such a test fails, besides disabling the test)

For LocalForage we aren't stringify anything as it just works, but we implement multiGet, multiMerge, multiSet as there are no such methods
We should verify they work as expected and produce the expected result

lib/storage/__mocks__/index.js

tgolen

It's looking very good to me at this stage!

kidroca · 2021-11-04T15:43:36Z

I've prepared the App PR with videos and retested everything: Expensify/App#4908
Found one issue on mobile web ios
Otherwise everything seems ok

I'm not sure how to proceed, here are some questions

should I remove the Draft / Hold state and mark the PR as ready?
should I write tests now?
or try to fix the mobile web problem first?

tgolen · 2021-11-04T17:51:41Z

I'd say it would be best to fix the mobile web problem first, write some super basic tests, then remove hold/draft and get it merged.

…

On Thu, Nov 4, 2021 at 9:43 AM Peter Velkov ***@***.***> wrote: I've prepared the App PR with videos and retested everything: Expensify/App#4908 <Expensify/App#4908> Found one issue on mobile *web* ios <Expensify/App#4908 (comment)> Otherwise everything seems ok I'm not sure how to proceed, here are some questions - should I remove the Draft / Hold state and mark the PR as ready? - should I write tests now? - or try to fix the mobile web problem first? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#102 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB7YATEX2ED5KILGX63UKKS3HANCNFSM5DB2YYTA> .

kidroca · 2021-11-12T14:36:12Z

@tgolen
I've found out what's causing the problem - the issues occurs because once we read the persisted requests we immediately delete them from storage, which also deletes file data
When we read a file from IndexedDB we receive only the metadata, we don't get the binary data - we receive a reference with the necessary hooks to read it if we want to. When we clear storage we lose the binary data. Here are more details: Expensify/App#4908 (comment)

I don't know why the issue occurs only on iOS Safari/Chrome, perhaps the other web platforms have some protection that doesn't delete data immediately

Any thought on how to proceed?

Since we've identified the problem I think we can continue and merge this the way it is. The offline file upload feature would work everywhere but mobile safari/chrome. Regular file upload still works there.
Or we can decide and apply an update to the offline request queue and then continue here

tgolen · 2021-11-12T15:35:12Z

I'm pretty hesitant to merge this with that big of a known bug. I'd like to see if we can fix it, or at least prevent the user from being affected (ie. disable the feature in those platforms until we fix the bug). I'm having trouble understanding the exact nature and reproduction steps of the bug though. Can you go into some more detail on why it happens and then maybe with me and @marcaaron we can try to figure something out.

the issues occurs because once we read the persisted requests we immediately delete them from storage, which also deletes file data

Does this happen because of changes in this PR, or because of localForage, or why? Can we control this at all to have it work better for us somehow? Where does this happen in the code?

kidroca · 2021-11-12T15:41:48Z

It's not something that this PR introduced, it's how the network queue works

The details are here: Expensify/App#4908 (comment) steps, code causing the issue and some ideas on how to address it

tgolen · 2021-11-12T16:20:48Z

Ah, perfect! Just what I needed. I'll comment over there.

kidroca · 2021-12-14T09:40:46Z

As per this comment: Expensify/App#4908 (comment) I've removed the draft status of this PR

thienlnam · 2021-12-14T18:39:48Z

@kidroca This shouldn't be on hold anymore right? Is this good to be reviewed/merged?

kidroca · 2021-12-14T19:54:37Z

Yes,
Nothing had actually changed here since the last review - but I found a way to get this merged - attachment persistence will remain disabled (in App) until the issue (in App) with the attachment getting deleted prematurely is resolved (working on that here: Expensify/App#6556)

As a summary:
Tim approved the PR, Marc left a comment but it's not a blocker: #102 (review)
We can open a follow up PR to add a few more tests

thienlnam · 2021-12-14T22:46:23Z

Cool cool, I took a look over the PR and things look good to me.

I opened up this issue as a reminder to add tests and add some documentation regarding these changes #112

Since the PR is approved by both Tim and Marc, I will merge to push this forward

NAB - opened another issue to address adding tests

kidroca added 26 commits August 26, 2021 01:29

Extract ProviderInterface

693684a

Captured existing storage interface from Onyx to separate file Storage provider mocking When we have provider.js and provider.native.js we need to have the mock implemented at the provider level instead of the AsyncStorage level

Add expo-file-system as peerDependency

535cf48

chore: add expo-file-system as dev dependency for intellisense

c8c8a61

Create storage/fs.native.js

95df46d

Create storage/fs.js for web and desktop

d77e1c5

Add localforage as peer dependency

b53c79a

Create LocalForage provider

e45b66a

this should allow us to store and retrieve Files as well as anything else from storage for web and desktop

Create AsyncStorage provider

55afdb2

This is the same Provider we had up until now but the JSON parsing and stringifying is capsulated here instead of Onyx

Move NativeFileHandler to providers

0513625

Remove JSON parse/stringify handling from Onyx

628c56d

This is handled by Storage providers

Update NativeFileHandler

42eab2a

Use FileSystem from unimodules as that's how to use expo's file system on a bare RN project Ad helper methods to check file items

Update Storage type definitions

355fa19

Remove fs.js - this is completely covered by localForage

661990a

Update package with unimodules

019ea8c

Update stringification

ef48097

NativeStorage implementation handling File items

86d8ad9

Simplify Native file save handling

20dd112

Instead of trying to guess and handle files leave this to happen explicitly by the library user

Simplify providers, move away from class based implementations

4448684

remove unused methods from FileHandler

c4441b5

rename "storeFileLocally" to "prepareFile"

449d802

Add documentation and expose through Onyx

Update prepareFile so it can be called sync

ec22e2c

Native Storage stats

577091f

Create folder for the local path

dcce126

iOS would not create the folder automatically on copy

Feat: synchronize IndexDB updates across different tabs

2968c30

Update tests after changes

0bffc81

Update docs

449e4ba

kidroca commented Aug 30, 2021

View reviewed changes

lib/Onyx.js Show resolved Hide resolved

lib/Onyx.js Show resolved Hide resolved

lib/Onyx.js Outdated Show resolved Hide resolved

lib/storage/index.native.js Outdated Show resolved Hide resolved

This was referenced Aug 30, 2021

Onyx File handling - preparation Expensify/App#4908

Merged

[Discussion] Add the ability to handle images in Onyx Expensify/App#3867

Closed

Use a hidden folder .onyx_files

b6ad244

kidroca added 3 commits November 3, 2021 22:23

Move storage/__mocks__ to the root level mocks

9b02f63

Small code style cleanup

3d5a675

package: remove localforage from dev dependencies

3c8aa3f

kidroca commented Nov 3, 2021

View reviewed changes

docs: fix typo

5e95175

kidroca requested review from tgolen and marcaaron November 3, 2021 21:32

tgolen approved these changes Nov 3, 2021

View reviewed changes

mateomorris mentioned this pull request Nov 24, 2021

[$250] [Image] Implement the ability to copy images to the clipboard Expensify/App#4629

Closed

kidroca marked this pull request as ready for review December 13, 2021 15:32

kidroca requested a review from a team as a code owner December 13, 2021 15:32

MelvinBot requested review from Gonals and removed request for a team December 13, 2021 15:33

thienlnam changed the title ~~[HOLD] File handling POC~~ File handling POC Dec 14, 2021

thienlnam approved these changes Dec 14, 2021

View reviewed changes

thienlnam mentioned this pull request Dec 14, 2021

Add tests and documentation for file handling changes #112

Closed

thienlnam merged commit 40c5b14 into Expensify:master Dec 14, 2021

parasharrajat mentioned this pull request Aug 10, 2022

[HOLD #10894][Image][Performance][$2000] Feature Request - Attachments should be cached after first load. It currently load from internet every time - Reported by: @parasharrajat Expensify/App#6527

Closed

thienlnam mentioned this pull request Oct 6, 2023

[$500] Creating an optimistic action with a default avatar (SVG component) causes a failed write to Onyx storage Expensify/App#28885

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File handling POC #102

File handling POC #102

kidroca commented Aug 30, 2021 •

edited

Loading

kidroca left a comment

kidroca left a comment

kidroca Nov 3, 2021

tgolen left a comment

kidroca commented Nov 4, 2021

tgolen commented Nov 4, 2021 via email

kidroca commented Nov 12, 2021

tgolen commented Nov 12, 2021

kidroca commented Nov 12, 2021

tgolen commented Nov 12, 2021

kidroca commented Dec 14, 2021

thienlnam commented Dec 14, 2021

kidroca commented Dec 14, 2021 •

edited

Loading

thienlnam commented Dec 14, 2021

		@@ -1,4 +1,4 @@
		import AsyncStorage from '@react-native-async-storage/async-storage';
		import Storage from '../../lib/storage';

File handling POC #102

File handling POC #102

Conversation

kidroca commented Aug 30, 2021 • edited Loading

Details

Uses localforage for web/desktop.

FIle saving on native

Related Issues

Automated Tests

Linked PRs

kidroca left a comment

Choose a reason for hiding this comment

kidroca left a comment

Choose a reason for hiding this comment

kidroca Nov 3, 2021

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

kidroca commented Nov 4, 2021

tgolen commented Nov 4, 2021 via email

kidroca commented Nov 12, 2021

tgolen commented Nov 12, 2021

kidroca commented Nov 12, 2021

tgolen commented Nov 12, 2021

kidroca commented Dec 14, 2021

thienlnam commented Dec 14, 2021

kidroca commented Dec 14, 2021 • edited Loading

thienlnam commented Dec 14, 2021

kidroca commented Aug 30, 2021 •

edited

Loading

kidroca commented Dec 14, 2021 •

edited

Loading