Add support for custom dataStore - Closes #209 #210

Redsandro · 2017-03-15T23:44:16Z

Now you can use a custom dataStore other than the default dataStore.

export default Ember.Route.extend(InfinityRoute, {
	customStore: Ember.inject.service('my-custom-store'),
	model(params) {
		return this.infinityModel('product', { perPage: 12, startingPage: 1, store: 'customStore' })
	}
});

Closes #209

Now you can use a custom dataStore other than the default dataStore. ```js export default Ember.Route.extend(InfinityRoute, { customStore: Ember.inject.service('my-custom-store'), model(params) { return this.infinityModel('product', { perPage: 12, startingPage: 1, store: 'customStore' }) } }); ```

Redsandro · 2017-03-16T00:05:41Z

@hhff I guess I broke something, huh?
Everything seems to work. I hypothesize that I broke the tests - not ember-infinity itself. I didn't touch the tests, but I see it uses the store parameter too. Perhaps it conflicts with the new store option somehow.

I don't really understand how these tests work. Can you take a look at this?

hhff · 2017-03-21T15:26:17Z

Thanks @Redsandro !

Do the tests run locally for you?

Redsandro · 2017-03-21T16:22:41Z

@hhff yes**, but I don't understand why the Travis tests fail. They say "The command "ember try $EMBER_TRY_SCENARIO test" exited with 1." before PhantomJS is even started.

**) All 76 tests run fine up to EmberJS 2.8. From 2.12, I'm starting to see errors that are irrelevant to my PR. I think.

default: PASS
ember-1.10: PASS
ember-1.11: PASS
ember-1.12: PASS
ember-1.13: PASS
ember-2.0: PASS
ember-2.1: PASS
ember-2.2: PASS
ember-2.3: PASS
ember-2.4: PASS
ember-2.5: PASS
ember-2.6: PASS
ember-2.7: PASS
ember-2.8: PASS
ember-release: FAIL
ember-release-ember-data-1.13.1x: FAIL
ember-beta: FAIL
ember-canary: FAIL
npm ERR! Test failed.  See above for more details.

For errors e.g. test 42, 43 and 44 on Ember 2.12:

For scenario ember-release-ember-data-1.13.1x, using:
Versions do not match: Expected: release but saw 2.12.0-release+69d2abe6 This might be ok, depending on the scenario
  ember 2.12.0-release+69d2abe6
Versions do not match: Expected: ~1.13.11 but saw 1.13.16 This might be ok, depending on the scenario
  ember-data 1.13.16
not ok 42 PhantomJS 2.1 - component:infinity-loader: it throws error when scrollable element is not found
    ---
        actual: >
            false
        expected: >
            true
        message: >
            Error: Ember Infinity: No scrollable element found for: #nonexistent
        Log: |
    ...
not ok 43 PhantomJS 2.1 - component:infinity-loader: it throws error when multiple scrollable elements are found
    ---
        actual: >
            false
        expected: >
            true
        message: >
            Error: Ember Infinity: Multiple scrollable elements found for: .hello
        Log: |
    ...
not ok 44 PhantomJS 2.1 - component:infinity-loader: it throws error when scrollable is something other than nothing or string
    ---
        actual: >
            false
        expected: >
            true
        message: >
            Error: Ember Infinity: Scrollable must either be a css selector string or left empty to default to window
        Log: |
    ...

Funny detail is that the error messages for the fails are doing what the test description sais. Perhaps the test is wrong, because the error is actually what is desired.

On Ember Beta half of the tests fail. But again, I think not relevant to this PR. I didn't touch css or selector functionality. Merely inserted option for datastore. I'm already using it. 😃

hhff · 2017-03-21T16:27:20Z

Got it - ok!

Would you try updating node version in the travis.yml file?

Redsandro · 2017-03-21T20:49:47Z

@hhff ah yes that's better. Now travis sees the same results as I.

hhff · 2017-03-21T21:36:13Z

Thanks @Redsandro - those final tests may be an issue with the state of Ember's release cycle. I've seen that happen a couple times. Will look into it!

snewcomer · 2017-04-07T00:32:17Z

This looks like a great PR! Looks like some of the tests are failing - same issue here: emberjs/ember-qunit#256

@hhff Any thoughts on next steps to getting these tests passing?

hhff · 2017-04-10T15:56:45Z

@Redsandro - can you please merge master, and ping @snewcomer when you're done? We've just fixed up all the testing weirdness 👍

snewcomer

@Redsandro @hhff Here are some thoughts on this PR! Let me know what you think.

Last idea would be to get some sort of test w/ a custom store. If you need help with this, I can give it a shot.

snewcomer · 2017-04-10T23:06:52Z

addon/mixins/route.js

@@ -151,7 +159,7 @@ const RouteMixin = Ember.Mixin.create({
      throw new Ember.Error("Ember Infinity: You are using an unsupported version of Ember Data.  Please upgrade to at least 1.13.4 or downgrade to 1.0.0-beta.19.2");
    }

-    if (Ember.isEmpty(this.get('store')) || Ember.isEmpty(this.get('store')[this._storeFindMethod])){
+    if (Ember.isEmpty(this._store) || Ember.isEmpty(this._store[this._storeFindMethod])){
      throw new Ember.Error("Ember Infinity: Ember Data store is not available to infinityModel");


@Redsandro could we potentially change this assertion to Store is not available...

I think that would be goldplating it. I assume one that creates and specifies a custom store knows what they're doing.

PS - I think I misunderstood your comment. 😊 You just want to change the error message. I leave that to you. Need to catch some 💤 before busy week starts!

snewcomer · 2017-04-10T23:12:06Z

addon/mixins/route.js

    const perPage      = options.perPage || this.get('_perPage');
+    const store        = options.store && this.get(options.store) || this.get('store');


We probably need to assert that options.store is a string in order to do this.get. If they pass an object or something, it might blow up on them!

snewcomer · 2017-04-10T23:13:35Z

addon/mixins/route.js

@@ -193,6 +199,7 @@ const RouteMixin = Ember.Mixin.create({
      currentPage: startingPage,
      _firstPageLoaded: false,
      _perPage: perPage,
+      _store: store,


@Redsandro We also probably need to be able to customize the _storeFindMethod method. What are your thoughts on this? Does your custom store use the same query method?

Also need to make sure that their method returns a promise as well. here

What are your thoughts on this? Does your custom store use the same query method?

My thoughts are that a custom store, just like a custom route or component, extends the Ember default (in this case Store) and as such is supposed to implement the same behavior as Ember.DS.

I don't see a use case where one would want to customuze the _storeFindMethod.

@Redsandro So here is one example. https://github.com/toranb/ember-cli-simple-store

I have created a custom functional store in the past and I have find/findAll methods that wouldn't work with this. Both of these just use plain Ember objects / JS objects as an identity map just like ED does (but muuuuuch simpler).

Interesting. I have never used a store that wasn't just Ember DS extended. Indeed this functionality is something that someone might want. But I'd consider using non-Ember-DS datastore with nondefault query mechanisms a separate PR/feature (request) once this is in.

Redsandro · 2017-04-10T23:40:11Z

~~@hhff @snewcomer done. 👍~~ shoot, didn't see reviews

Redsandro · 2017-04-11T00:08:55Z

@snewcomer said:

Last idea would be to get some sort of test w/ a custom store.

I don't understand how the store works in tests, and I have no clue how that would work with a custom store. Writing a custom store in an Ember app is as simple as:

ember g service custom-store

import DS from 'ember-data';

export default DS.Store.extend({});

However, you probably want a custom store to have a different adapter and/or serializer than the default store.

ember g service custom-store
ember g adapter custom-store
ember g serializer custom-store

service:

import DS from 'ember-data';

export default DS.Store.extend({
	adapterFor() {
		return this._super('custom-store');
	},
	serializerFor() {
		return this._super('custom-store');
	}
});

You must then inject the store to your route before the infinityModel can use it:

export default Ember.Route.extend(InfinityRoute, {
	customStore: Ember.inject.service('custom-store'),
	model(params) {
		return this.infinityModel('product', { perPage: 12, startingPage: 1, store: 'customStore' })
	}
});

Redsandro · 2017-04-11T00:11:06Z

Oh great I broke it.

Redsandro · 2017-04-11T00:18:54Z

I have added a check for the string type. As I am currently on a time deficit, I would propose that this is safe to merge as is, and you can take a stab at writing a test.

snewcomer · 2017-04-11T00:21:26Z

@Redsandro Awesome. Thanks for putting in the hard work! Yep I see where the disconnect is :). So yeah I was hoping this PR could handle both a custom store like you defined, but also any service (w/ a local cached array of JS objects) that wishes to request data from a backend and update that cache...not just ED specific. But now seeing that may be out of scope for this PR and I can tackle in a separate PR.

Redsandro · 2017-04-11T00:27:37Z

@snewcomer very well! 👍

snewcomer · 2017-04-11T04:33:55Z

@Redsandro Could you either:

Invite me as a collaborator under the settings section of your fork? Just some small changes I wanted to push up and show you so we can get this PR in!
I also created a branch called develop. You would then merge your code into that branch and I will offer up the suggestions there. Your commit will be the final merge commit still.

Your call!

Redsandro · 2017-04-11T09:56:01Z

@snewcomer sorry for the delay. I keep forgetting Github doesn't automatically add the owners of the original repo as collaborators to the fork. I've invited you manually.

I'm sorry I haven't been more accomodating in tailoring this PR. I'm very thankful you're taking the time to merge this properly.

If you haven't been notified by email, let me know and I will send you the invitation link.

snewcomer · 2017-04-12T00:39:34Z

@Redsandro Here are some small minor changes for your review, branched off of your patch-1 branch. It mainly revolves around changing the type of the _store property to a String. Let me know your thoughts and can merge into your patch-1 branch if you would like.

Redsandro@30416cb

snewcomer · 2017-04-12T00:46:10Z

As you probably already know, customStore: service('my-custom-store') is useful in regards to this PR if you have multiple stores defined on a route. Otherwise, you can just do store: service('my-custom-store'). So wanted to limit the places we were dealing with the options.store key here since it is usually the case one store lives on a route.

Super happy you put in this PR! 👍

small improvements. Review and merge into your branch if you would like

Redsandro · 2017-04-12T02:30:22Z

@snewcomer Looking good. I couldn't follow the concequences of the changes by thought-simulation alone so I waited until I had the time to try this PR in an app with full custom store using dependency "ember-infinity": "Redsandro/ember-infinity#30416cbd9c8bed88356b15fde2f9d5a030cf1949", and it works fine. Nice job! 👍

snewcomer · 2017-04-12T14:34:09Z

@hhff for your review.

hhff · 2017-04-12T14:44:42Z

Awesome - thankyou so much @Redsandro & @snewcomer !

tradziej · 2017-05-26T12:29:20Z

@hhff It seems it is merged to master but not released at latest 0.2.8 version. When it is possible to release it? It is confusing to read about it on README but not available at latest release.

hhff · 2017-05-26T14:41:02Z

Hi @tradziej - @snewcomer is in the process of working on the 1.0 release of Ember Infinity, and the master branch is in a state of development for now, so we're holding off new releases for that reason.

I agree it's confusing and we're getting close to a beta version - for now if that's a feature you need, please use the git tag for that feature merge in your package.json - 0cacddd69422312e4689eee44f69892de8d60e51

snewcomer · 2017-05-26T17:53:37Z

@tradziej yeah hopefully sometime next week we can release a new version!

Update travis.yml node version

d1dd611

Update README

a3bf535

Merge branch 'master' into patch-1

d91db9d

snewcomer requested changes Apr 10, 2017

View reviewed changes

assert that options.store is a string

ae47d93

Oopsie options not defined until later

58f5405

small improvements. Review and merge into your branch if you would like

30416cb

Merge pull request #1 from Redsandro/scott-fixes

a92216b

small improvements. Review and merge into your branch if you would like

snewcomer approved these changes Apr 12, 2017

View reviewed changes

hhff merged commit 0cacddd into adopted-ember-addons:master Apr 12, 2017

Redsandro deleted the patch-1 branch April 12, 2017 15:40

		const perPage = options.perPage \|\| this.get('_perPage');
		const store = options.store && this.get(options.store) \|\| this.get('store');

Add support for custom dataStore - Closes #209 #210

Add support for custom dataStore - Closes #209 #210

Conversation

Redsandro commented Mar 15, 2017 • edited Loading

Redsandro commented Mar 16, 2017 • edited Loading

hhff commented Mar 21, 2017

Redsandro commented Mar 21, 2017 • edited Loading

hhff commented Mar 21, 2017

Redsandro commented Mar 21, 2017

hhff commented Mar 21, 2017

snewcomer commented Apr 7, 2017

hhff commented Apr 10, 2017

snewcomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Redsandro commented Apr 10, 2017 • edited Loading

Redsandro commented Apr 11, 2017

Redsandro commented Apr 11, 2017

Redsandro commented Apr 11, 2017

snewcomer commented Apr 11, 2017

Redsandro commented Apr 11, 2017

snewcomer commented Apr 11, 2017

Redsandro commented Apr 11, 2017

snewcomer commented Apr 12, 2017 • edited Loading

snewcomer commented Apr 12, 2017 • edited Loading

Redsandro commented Apr 12, 2017 • edited Loading

snewcomer commented Apr 12, 2017

hhff commented Apr 12, 2017

tradziej commented May 26, 2017

hhff commented May 26, 2017

snewcomer commented May 26, 2017

Redsandro commented Mar 15, 2017 •

edited

Loading

Redsandro commented Mar 16, 2017 •

edited

Loading

Redsandro commented Mar 21, 2017 •

edited

Loading

snewcomer Apr 11, 2017 •

edited

Loading

Redsandro commented Apr 10, 2017 •

edited

Loading

snewcomer commented Apr 12, 2017 •

edited

Loading

snewcomer commented Apr 12, 2017 •

edited

Loading

Redsandro commented Apr 12, 2017 •

edited

Loading