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

Localstorage working #1

Merged
merged 14 commits into from Feb 25, 2016
Merged

Localstorage working #1

merged 14 commits into from Feb 25, 2016

Conversation

@ekryski
Copy link
Member

ekryski commented Feb 23, 2016

This gets all the tests passing (server side) and adds examples, etc. In theory because the node module api is the same as the browser it should work just fine there too.

@@ -19,19 +19,27 @@ class LocalStorage extends Service {
}

create(... args) {
return super.create(... args).then(this.flush);
return super.create(... args).then(data => {
return this.flush.call(this, data);

This comment has been minimized.

Copy link
@daffl

daffl Feb 23, 2016

Member

Shorthand: return super.create(... args).then(data => this.flush(data));

This comment has been minimized.

Copy link
@ekryski

ekryski Feb 23, 2016

Author Member

I'm pretty sure I tried that and it didn't work. I'll double check.


describe('feathers-localstorage', () => {
it('is CommonJS compatible', () => {

This comment has been minimized.

Copy link
@daffl

daffl Feb 23, 2016

Member

We might want to set up after() to make sure that something has been written to the storage:

after(done => {
  setTimeout(() => {
    const store = JSON.parse(storage.getItem('feathers'));
    if(!store || !Object.keys(store).length) {
     return done(new Error('Nothing in storage'));
    }
    done();
  }, 200);
});

This comment has been minimized.

Copy link
@ekryski

ekryski Feb 23, 2016

Author Member

Not sure I follow. Shouldn't the tests already be testing that stuff was written to storage?

@ekryski
ekryski reviewed Feb 24, 2016
View changes
src/index.js Outdated
ready() {
if(!this.store) {
return Promise.resolve(this.storage.getItem(this.storageKey))
.then(str =>JSON.parse(str || '{}'))

This comment has been minimized.

Copy link
@ekryski

ekryski Feb 24, 2016

Author Member

I think we might have to have a try catch here in case someone pre-loads their localstorage data with invalid JSON.

@daffl daffl force-pushed the localstorage-working branch to 605e911 Feb 24, 2016
@ekryski

This comment has been minimized.

Copy link
Member Author

ekryski commented Feb 24, 2016

feathersjs-ecosystem/errors#26 will need to be landed and feathers-memory will need to be updated to the next major feathers-errors version before this can be merged.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Feb 24, 2016

Good thing is that feathers-memory will only be a patch release. Plus we can change the Lodash dependencies so that it doesn't pull in the whole thing.

@ekryski

This comment has been minimized.

Copy link
Member Author

ekryski commented Feb 25, 2016

Still need to get a new version of feathers-memory in without all of lodash before we merge this. feathersjs-ecosystem/feathers-memory#18

ekryski added a commit that referenced this pull request Feb 25, 2016
Localstorage working
@ekryski ekryski merged commit 2d12790 into master Feb 25, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ekryski ekryski deleted the localstorage-working branch Feb 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.