Skip to content
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.

Sign up for GitHub

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

Switch async-storage repository #85

Merged
merged 5 commits into from
Jul 9, 2021
Merged

Switch async-storage repository #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0227177
chore: switch async-storage repository
kidroca Jul 5, 2021
fccab2f
chore: add @react-native-async-storage/async-storage as peer dependency
kidroca Jul 6, 2021
fbccb45
chore: also include @react-native-async-storage/async-storage as dev …
kidroca Jul 6, 2021
a1ec213
refactor: update @react-native-async-storage references in tests
kidroca Jul 6, 2021
064748f
fix: calling `multiSet` with an empty array throws an exception
kidroca Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions __mocks__/@react-native-async-storage/async-storage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export {default} from '@react-native-async-storage/async-storage/jest/async-storage-mock';
1 change: 0 additions & 1 deletion __mocks__/@react-native-community/async-storage.js
View file
Open in desktop

This file was deleted.

15 changes: 11 additions & 4 deletions lib/Onyx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import _ from 'underscore';
import AsyncStorage from '@react-native-community/async-storage';
import AsyncStorage from '@react-native-async-storage/async-storage';
import Str from 'expensify-common/lib/str';
import lodashMerge from 'lodash/merge';
import {registerLogger, logInfo, logAlert} from './Logger';
Expand Down Expand Up @@ -675,18 +675,25 @@ function mergeCollection(collectionKey, collection) {
const keyValuePairsForExistingCollection = prepareKeyValuePairsForStorage(existingKeyCollection);
const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection);

const promises = [];

// New keys will be added via multiSet while existing keys will be updated using multiMerge
// This is because setting a key that doesn't exist yet with multiMerge will throw errors
const existingCollectionPromise = AsyncStorage.multiMerge(keyValuePairsForExistingCollection);
const newCollectionPromise = AsyncStorage.multiSet(keyValuePairsForNewCollection);
if (keyValuePairsForExistingCollection.length > 0) {
promises.push(AsyncStorage.multiMerge(keyValuePairsForExistingCollection));
}

if (keyValuePairsForNewCollection.length > 0) {
promises.push(AsyncStorage.multiSet(keyValuePairsForNewCollection));
}
Comment on lines +686 to +688
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the update AsyncStorage.multiSet throws when it is called with an empty array


// Merge original data to cache
cache.merge(collection);

// Optimistically inform collection subscribers
keysChanged(collectionKey, collection);

return Promise.all([existingCollectionPromise, newCollectionPromise])
return Promise.all(promises)
.catch(error => evictStorageAndRetry(error, mergeCollection, collection));
});
}
Expand Down
13 changes: 8 additions & 5 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
"test": "jest"
},
"dependencies": {
"@react-native-community/async-storage": "^1.12.1",
"expensify-common": "git+https://github.com/Expensify/expensify-common.git#2e5cff552cf132da90a3fb9756e6b4fb6ae7b40c",
"lodash": "4.17.21",
"react": "^16.13.1",
Expand All @@ -24,6 +23,7 @@
"@babel/preset-env": "^7.11.0",
"@babel/preset-react": "^7.10.4",
"@babel/runtime": "^7.11.2",
"@react-native-async-storage/async-storage": "^1.15.5",
"@react-native-community/eslint-config": "^2.0.0",
"@testing-library/jest-native": "^3.4.2",
"@testing-library/react-native": "^7.0.2",
Expand All @@ -41,6 +41,9 @@
"react-test-renderer": "16.13.1",
"metro-react-native-babel-preset": "^0.61.0"
},
"peerDependencies": {
"@react-native-async-storage/async-storage": "^1.15.5"
Copy link
Collaborator

@tgolen tgolen Jul 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never used peerDependencies before. What is it for and is it necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put that into the description but here's some more information

AsyncStorage is linked to native ios/android through the project that uses it - E.cash
You can set some gradle properties and settings there and this cannot work if it's not a part of E.cash

It sounds counterintuitive to provide a dependency to a package you're installing but this addresses problems like 2 or more different version of AsyncStorage being installed

You need AsyncStorage in E.cash so you can configure it and link it to the app
As you've seen E.cash and Onyx actually declared different version of AsyncStorage
To avoid this problem only one package should install the dependency
When npm install installs react-native-onyx it would hook it to the same version of AsyncStorage installed for the project

Usually libraries need to work together with other libraries that use the same packages, sometimes they even need to share the same instance of a module as well that's why they leave the project that installs them to resolve their peer dependencies

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, that's really interesting. Funny that I've never ran into this before, but ReactNative is a bit of a different monster that I'm still getting used to. Thanks for the information!

},
"jest": {
"preset": "react-native",
"transform": {
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/cacheEvictionTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import AsyncStorage from '@react-native-community/async-storage';
import AsyncStorage from '@react-native-async-storage/async-storage';
import Onyx from '../../index';
import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';

Expand Down
2 changes: 1 addition & 1 deletion tests/unit/onyxCacheTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ describe('Onyx', () => {
const OnyxModule = require('../../index');
Onyx = OnyxModule.default;
withOnyx = OnyxModule.withOnyx;
AsyncStorageMock = require('@react-native-community/async-storage').default;
AsyncStorageMock = require('@react-native-async-storage/async-storage').default;
cache = require('../../lib/OnyxCache').default;

Onyx.init({
Expand Down