-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Onyx File handling - preparation #4908
Conversation
LocalForage fails to save the value because functions cannot be serialized
Remove the `!isAttachment` rule as now we can persist Files
The lock file was update after expo-file-system was uninstalled
Synchronize tab functionality is moved to Onyx
9e64fec
to
90e0925
Compare
90e0925
to
3ea51ec
Compare
# Conflicts: # ios/Podfile.lock # package.json # src/components/AttachmentPicker/index.native.js # src/components/OnyxProvider.js # src/libs/listenToStorageEvents/index.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention again that registerStorageEventListener
is no longer needed as it's covered internally in Onyx
registerStorageEventListener: (onStorageEvent) => { | ||
listenToStorageEvents(onStorageEvent); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Synchronizing storage between tabs is handled internally by Onyx as it depends on the underlying storage implementation.
See: https://github.com/kidroca/react-native-onyx/blob/b6ad24440fe0b07b314373c1a4cfdf7914febc3f/lib/storage/index.js#L10-L15
# Conflicts: # android/app/src/main/java/com/expensify/chat/generated/BasePackageList.java # ios/Podfile.lock # package-lock.json # package.json # src/setup/index.js
Onyx no longer needs this Removing `prepareFile` usage
The document picker makes the attachment available until the application is terminated To persist Document Attachments and be able to use them on the next app launch we need to move them to the cache dir This way we can persist a reference for the attachment in Onyx and support offline files The `ImagePicker` library is doing this by default
There seems to be some problem and nothing happens when image is selected or when an image is shot
This fixes the issue with the image picker not working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the Document/Image picker libraries in an attempt to solve an issue I had
It turned it it wasn't caused by these libraries, but I suppose now it's a good time to update them - QA will use the Attachment picker to test the changes made here anyway
@@ -65,7 +66,7 @@ function getDataForUpload(fileData) { | |||
return { | |||
name: fileData.fileName || fileData.name || 'chat_attachment', | |||
type: fileData.type, | |||
uri: fileData.uri, | |||
uri: fileData.fileCopyUri || fileData.uri, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The document picker makes the attachment available until the application is terminated
To persist Document Attachments and be able to use them on the next app launch we need to move them to the cache dir
This way we can persist a reference for the attachment in Onyx and support offline files
The ImagePicker
library is already making a copy in cache by default
* @param {ImagePickerResponse|DocumentPickerResponse} attachment | ||
* @param {Array<ImagePickerResponse|DocumentPickerResponse>} attachments | ||
*/ | ||
pickAttachment(attachment) { | ||
if (attachment) { | ||
if (attachment.width === -1 || attachment.height === -1) { | ||
this.showImageCorruptionAlert(); | ||
return; | ||
} | ||
const result = getDataForUpload(attachment); | ||
this.completeAttachmentSelection(result); | ||
pickAttachment(attachments = []) { | ||
if (attachments.length === 0) { | ||
return; | ||
} | ||
|
||
const fileData = _.first(attachments); | ||
|
||
if (fileData.width === -1 || fileData.height === -1) { | ||
this.showImageCorruptionAlert(); | ||
return; | ||
} | ||
|
||
const result = getDataForUpload(fileData); | ||
this.completeAttachmentSelection(result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated document picker responds with an array. The image picker would also respond with array that's why this was changed to accept an array value
# Conflicts: # package-lock.json # package.json
a2459be
to
555d9ae
Compare
The Pod file already has this version of Flipper and pipelines are failing due to the mismatch
Hey @tgolen sorry just pushed an update to fix the pipeline |
I think we forgot to remove the [HOLD] label here
cc @thienlnam |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review, just a couple questions
// Notifies all tabs that they should sign out and clear storage. | ||
SHOULD_SIGN_OUT: 'shouldSignOut', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just not used? Not seeing it in any changes from this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was used here: src/libs/SignoutManager.js
But now the file is deleted - no longer necessary
What the SignoutManager
did was to synchronize the logout across multiple tabs
But now with localForage
this is no longer needed
We're touching on this here: Expensify/react-native-onyx#102 (comment)
@@ -53,6 +53,7 @@ function getImagePickerOptions(type) { | |||
*/ | |||
const documentPickerOptions = { | |||
type: [RNDocumentPicker.types.allFiles], | |||
copyTo: 'cachesDirectory', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does android have a cachesDirectory? I only see it for IOS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation says:
[iOS and Android only] copyTo:"cachesDirectory" | "documentDirectory"
https://www.npmjs.com/package/react-native-document-picker#ios-and-android-only-copytocachesdirectory--documentdirectory
I take it the option copyTo
and the value cachesDirectory
is available in both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @thienlnam, I hope these answer your questions well
// Notifies all tabs that they should sign out and clear storage. | ||
SHOULD_SIGN_OUT: 'shouldSignOut', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was used here: src/libs/SignoutManager.js
But now the file is deleted - no longer necessary
What the SignoutManager
did was to synchronize the logout across multiple tabs
But now with localForage
this is no longer needed
We're touching on this here: Expensify/react-native-onyx#102 (comment)
@@ -53,6 +53,7 @@ function getImagePickerOptions(type) { | |||
*/ | |||
const documentPickerOptions = { | |||
type: [RNDocumentPicker.types.allFiles], | |||
copyTo: 'cachesDirectory', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation says:
[iOS and Android only] copyTo:"cachesDirectory" | "documentDirectory"
https://www.npmjs.com/package/react-native-document-picker#ios-and-android-only-copytocachesdirectory--documentdirectory
I take it the option copyTo
and the value cachesDirectory
is available in both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for those comments, this looks all good on my end 👍 Thanks for those comments, this looks all good on my end 👍
🤦♂️ It seems this closed another PR due to the added description:
|
🚀 Deployed to staging by @thienlnam in version: 1.1.29-7 🚀
|
There is a deploy blocker causing that PDF attachment shows an infinite spinner on Android (only as far as the QA team found). This PR seems like the only one in the latest deployment, which is related. Would @kidroca be able to look into this and confirm if it is related? If yes, would we revert this PR or would you be able to identify a fix to CP it soon? Thank you for your help! 🙇 |
I'll investigate |
BTW the current PR applies a migration: https://github.com/Expensify/App/pull/4908/files#diff-fc59a9d775dba3004d8ed5a4dec154bbd2487455e7c720afed29bc8003a5228f Reverting the PR won't revert the migration and the result would be users getting logged out when they open the app IMO we should not revert, but apply #7277 |
@kidroca Thank you very much for looking into this. We should indeed be careful with migrations like this. It might not be very common to run a migration, but maybe it might be worth making PRs like this clearly state it. For example, the PR could have a |
I like the idea of adding that tag try avoid reverting a PR with a
migration. We could use a label too (which would probably be better).
…On Mon, Jan 17, 2022 at 5:34 AM Vit Horacek ***@***.***> wrote:
@kidroca <https://github.com/kidroca> Thank you very much for looking
into this. We should indeed be careful with migrations like this. It might
not be very common to run a migration, but maybe it might be worth making
PRs like this clearly state it.
For example, the PR could have a [Migration] prefix so then every
engineer will immediately know it might not be easy to revert the PR. What
do you think, @tgolen <https://github.com/tgolen> and @kidroca
<https://github.com/kidroca>?
—
Reply to this email directly, view it on GitHub
<#4908 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMAB2HIPSYIXVETSE6XSLUWQEFDANCNFSM5DB5KQ2A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
🚀 Deployed to production by @roryabraham in version: 1.1.30-3 🚀
|
cc @thienlnam
Details
This is a proof of concept work that showcases file handling in Onyx as per the linked issue
Onyx was updated with peer dependency to localforage
Onyx PR: Expensify/react-native-onyx#102
Deleted
listenToStorageEvents
lib - this is now handled using IndexedDb andlocalforage
in OnyxDeleted
SignOutManager
lib - the updatedOnyx.clear
logic is synchronized across tabs and makes it unnecessaryFixed Issues
$ #3867
Tests
#### For all platforms1. Go offline2. Open a conversation
3. Select and send an attachment
4. Terminated the app
5. Resume connectivity
6. Open the app again
7. Go to the conversation and see the file appearing after it's uploaded
- depending on how big is the file or how fast you do the last step - the file might already be uploaded
Edit: The change needed for this was reverted
It will be added and persisting attachments would work after we resolve: #6556
Regression check for Web
Multiple Tabs
Multiple Tabs (Signing out)
QA Steps
Tested On
Screenshots
Web
No regressions on persisting plain messages
New.Expensify.-.Google.Chrome.2021-12-15.01-57-51.mp4
Using multiple tabs/windows
2021-11-03_22-05-01.mp4
Mobile Web
Screen.Recording.2021-12-15.at.2.19.10.mov
Desktop
Screen.Recording.2021-12-15.at.2.12.51.mov
iOS
Screen.Recording.2021-12-15.at.2.15.09.mov
Android
Android.Emulator.-.Pixel_2_API_29_5554.2021-12-15.02-29-11.mp4