-
-
Notifications
You must be signed in to change notification settings - Fork 594
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
LocalDatastore fixes for React-Native #753
Conversation
Codecov Report
@@ Coverage Diff @@
## master #753 +/- ##
==========================================
+ Coverage 90.25% 90.38% +0.12%
==========================================
Files 54 55 +1
Lines 4640 4709 +69
Branches 1077 1084 +7
==========================================
+ Hits 4188 4256 +68
- Misses 452 453 +1
Continue to review full report at Codecov.
|
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.
some questions.....
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.
some questions and comments. i think going over the types in comments and flow types in function declarations with a fine tooth comb is in order.
@dplewis lmk or assign to me when ready for review. |
Its ready for review |
looking, but you'll need to rebase. |
let's see if i can just push the button to update. |
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.
I found one error which requires a fix which I point out in the comment.
I think that the rest of my comments you can take or leave.
Working this with you has been good. Hope it isn't too painful :).
We should do this again sometime :) |
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.
an actual question. and a suggestion or two to take or leave.
i'm going to have to find a use parse in the client now, cause I wanna use this!
const fromPinPromise = this.fromPinWithName(pinName); | ||
const [pinned] = await Promise.all([fromPinPromise, toPinPromises]); | ||
const toPin = [...new Set([...(pinned || []), ...objectKeys])]; | ||
return this.pinWithName(pinName, toPin); | ||
}, |
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.
take 2 at using reduce here, this time with your code. Untested as I'm not setup to test. Just for fun,
async _handlePinAllWithName(name: string, objects: Array<ParseObject>): Promise {
const pinName = this.getPinName(name);
const toPinPromises = objects.reduce((accumulator, parent) => {
const toPin = this._getChildren(parent); // get an object with the children by key
const parentKey = this.getKeyForObject(parent);
toPin[parentKey] = parent._toFullJSON(); // add the parent
const promises = Object.keys(toPin).map(objectKey =>
this.pinWithName(objectKey, [toPin[objectKey]])) // pin 'em.
.then(() => objectKey) // return the object key when the promise is done
);
return accumulator.concat(promises);
}, []);
const fromPinPromise = this.fromPinWithName(pinName);
const [fromPins, toPins] = await Promise.all([fromPinPromise, toPinPromises]);
const allToPin = [...new Set([...(fromPins || []), ...toPins])];
return this.pinWithName(pinName, allToPin);
},
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.
I tried that but for some reason to get the toPins I would have to call Promise.all
twice
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.
I'd have to be able to step through this with a debugger to see.
One thing I'm curious about is why this.pinWithName(objectKey, [toPin[objectKey]]))
I'm assuming that toPin[objectKey]
is already an array, so not sure why it needs to be put in an array.
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.
getChildren returns key -> object and not key -> [object]
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.
Having another pair of eyes debugging this is really helpful
const keys = await RNStorage.getAllKeys(); | ||
const storage = {}; | ||
const results = await RNStorage.multiGet(keys); | ||
results.map((pair) => { |
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.
can use forEach here instead of map cause you're not capturing the returned array.
map returns an array with one element per element in the original array
forEach return nothing.
so forEach is more memory efficient if you're not returning something.
map composes very nicely with filter.
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.
👍
* ⚡️ Release v2.3.0 Closes: #687, #670, #723, #669 * Pin packages * (Jest) Fix deprecated setupTestFrameworkScriptFile * (Codecov) Fix coverage folders * Update Parse.Cloud.define docs * Add discource badge to Readme Pending #753 * add user subclass fix * add typescript to readme * typo * add discourse to js sdk * remove url module #758 (review) * Update CHANGELOG.md
@dplewis I just want to say thank you and the whole Parse contributors for your efforts! |
Closes: #747
Adds a new Prefix on stored object keys. This prevents retrieving and clearing unnecessary objects in the redux data store.