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.

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

Local Datastore #612

Merged
merged 48 commits into from
Dec 24, 2018
Merged

Local Datastore #612

merged 48 commits into from
Dec 24, 2018

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Jul 27, 2018

This is based on discussion #77

This is a Work In Progress.

@codecov
Copy link

codecov bot commented Jul 27, 2018

Codecov Report

Merging #612 into master will increase coverage by 2.88%.
The diff coverage is 98.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #612      +/-   ##
==========================================
+ Coverage   85.69%   88.57%   +2.88%     
==========================================
  Files          48       53       +5     
  Lines        3928     4535     +607     
  Branches      898     1052     +154     
==========================================
+ Hits         3366     4017     +651     
+ Misses        562      518      -44
Impacted Files Coverage Δ
src/Parse.js 84.61% <100%> (+4.01%) ⬆️
src/equals.js 100% <100%> (+8%) ⬆️
src/StorageController.react-native.js 100% <100%> (+15.78%) ⬆️
src/CoreManager.js 100% <100%> (ø) ⬆️
src/LocalDatastoreController.browser.js 100% <100%> (ø)
src/LocalDatastoreController.default.js 100% <100%> (ø)
src/LocalDatastoreController.react-native.js 100% <100%> (ø)
src/ParseQuery.js 95.75% <95.94%> (+0.93%) ⬆️
src/ParseObject.js 89.47% <98.59%> (+0.73%) ⬆️
src/LocalDatastore.js 98.75% <98.75%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b6f363...51ac2f1. Read the comment docs.

@dplewis
Copy link
Member Author

dplewis commented Aug 1, 2018

@flovilmart Can you check Travis for me? I've been having gulp issues.

Also can you do a quick run though of what I've done so far? Its pretty close to being done.

@flovilmart
Copy link
Contributor

Error: Cannot find module 'utf-8-validate' from '/home/travis/build/parse-community/Parse-SDK-JS/node_modules/parse/node_modules/ws/lib'

It’s the error that I see before the integration tests, during the browserify build

@dplewis
Copy link
Member Author

dplewis commented Aug 1, 2018

@flovilmart I got both

Error: Cannot find module 'utf-8-validate' from 'xxx/node_modules/ws/lib'
Error: Cannot find module 'bufferutil' from 'xxx/node_modules/ws/lib'

I tried to npm install --save them but ended up with another error

@flovilmart
Copy link
Contributor

Uhm, that’s odd, try reverting ws to an earlier version then :)

@dplewis
Copy link
Member Author

dplewis commented Aug 2, 2018

@flovilmart I fixed the issue. It was a lot of circular dependency. Since localStorage isn't available I'll have to mock it along with some other tests.

Can you have a quick look through?

@flovilmart
Copy link
Contributor

I’ll have a look. The diff hit for coverage is only 21.11% can we do something?

@dplewis
Copy link
Member Author

dplewis commented Aug 2, 2018

@flovilmart I'll try

@@ -41,29 +40,30 @@ const LocalDatastore = {
controller.clear();
},

_handlePinWithName(name: string, object: ParseObject) {
_handlePinWithName(name: string, object: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why loose the typing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept running into a

Uncaught TypeError: Super expression must either be null or a function, not undefined

Whenever I used ParseObject in the tests. I found a fix for it so I'll add the typing back.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are 💯

@flovilmart
Copy link
Contributor

I’ll review over the weekend

});
});

describe('Parse Query Pinning (LocalStorage)', () => {
Copy link
Member Author

@dplewis dplewis Aug 9, 2018

Choose a reason for hiding this comment

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

@flovilmart Quick question? Since I have both in-memory and localStorage available. I want to run the test cases against both without copying them. Would putting describe inside of a for-loop make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep it works, otherwise what you can do is put it into a functions and call the function from the specific describe

Copy link
Member Author

Choose a reason for hiding this comment

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

I just tried putting it in a loop. It also works great with Jasmine Test Explorer too

@dplewis
Copy link
Member Author

dplewis commented Sep 21, 2018

@flovilmart @acinader Quick Update

Since objects have serverData some of the querying from LDS may not work as intended

query.select() the keys are always present because of server data

object.fetchFromLocalDatastore it currently overrides server data, I'll remove it but that means if you do something like object.set('field', 'will not override') that field will still be set after fetch

object.fetchFromLocalDatastore I'm stuck on the recursion portion of it. I started it but its incomplete. The test is called can fetchFromLocalDatastore break multiple cycle in ParseLocalDatastoreTest.js. If you have time can you look into it?

improve coverage and clean up unused code
@flovilmart
Copy link
Contributor

@dplewis thanks! I'll have a look!

@dplewis dplewis requested a review from acinader October 4, 2018 01:01
Copy link
Contributor

@acinader acinader left a comment

Choose a reason for hiding this comment

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

I've read through it all. This looks good to me and I can see it much requested.

Do you think this would be useful in node? cloud code?

@acinader
Copy link
Contributor

acinader commented Oct 4, 2018

Could be helpful at this point to make a draft section to go in the guide? I'd be glad to help edit and that could help me try this out too?

@dplewis
Copy link
Member Author

dplewis commented Oct 4, 2018

@acinader I opened a PR in the docs repo parse-community/docs#558

@acinader
Copy link
Contributor

acinader commented Oct 4, 2018

man, that's seriously helpful. I have barely touched the IOS SDK, so this makes more sense now.

@flovilmart
Copy link
Contributor

Where do we stand on this one?

@dplewis
Copy link
Member Author

dplewis commented Dec 16, 2018

It should be good to go!

@flovilmart
Copy link
Contributor

Is it completely opt-in? And anyone not wanting the feature should be able to experience no regression?

@dplewis
Copy link
Member Author

dplewis commented Dec 16, 2018

Yes you have to enable it with Parse.enableLocalDatastore().

@flovilmart
Copy link
Contributor

I’ll give a good read and let you merge

Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

Let’s see how it goes!

@dplewis dplewis merged commit 629a550 into parse-community:master Dec 24, 2018
@dplewis dplewis deleted the LDS branch December 24, 2018 01:54
@GoMino
Copy link

GoMino commented Feb 11, 2019

Hello guys does updating an object while being offline, get sync automatically when network access reappear with this new feature (react-native usage) ?

Can't wait to test the localstorage feature with the upcoming 2.2.0 release 🤩

@dplewis
Copy link
Member Author

dplewis commented Feb 11, 2019

It currently doesn't exist. Thats something we can implement in the future. I want to play around with this in production for a bit. Feel free to open a PR.

@GoMino
Copy link

GoMino commented Feb 12, 2019

Ok so if I understand correctly this version smoothly enable offline read access to already sync datas.
I will let you know if I have time to add support for write access.

@GoMino
Copy link

GoMino commented Feb 13, 2019

@dplewis is the following doc regarding localstorage (parse-community/docs#558) up to date with the last 2.2.0 release ?

@dplewis
Copy link
Member Author

dplewis commented Feb 13, 2019

Yes, I also opened a PR to address your sync feature. Waiting for 2.2.0 to be release to npm

@GoMino
Copy link

GoMino commented Feb 13, 2019

Great I will have a look at your pull request #734 when I have time!
First I will test the 2.2.0 release to understand how this new feature is behaving (for offline read only access)...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants