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

Register the store as a service #2820

Merged
merged 1 commit into from Mar 7, 2015
Merged

Conversation

@martndemus
Copy link
Contributor

martndemus commented Feb 25, 2015

I propably want to add a test.
And also people want to feedback on this.
#2818

@@ -33,5 +33,6 @@ export default function initializeStore(container, application) {

// Eagerly generate the store so defaultStore is populated.
// TODO: Do this in a finisher hook
container.lookup('store:main');
var store = container.lookup('store:main');
container.register('service:store', store, { instantiate: false });

This comment has been minimized.

Copy link
@bmac

bmac Feb 25, 2015

Member

Travis failed because there are 2 spaces after store, which made jscs unhappy.

This comment has been minimized.

Copy link
@martndemus

martndemus Feb 25, 2015

Author Contributor

I'll fix it :)

@martndemus
Copy link
Contributor Author

martndemus commented Feb 25, 2015

The Ember used in the integration tests is 1.9.1, so I can't add a test to check if the actual injection works. (Though it works on my application with Ember 1.10.0)

@bmac
Copy link
Member

bmac commented Feb 25, 2015

On travis tests get run against the latest release, beta and canary channels. If you are testing locally you can change the ember version using the emberchannel query parameter (e.g. http://localhost:4200/?hidepassed&emberchannel=beta or http://localhost:4200/?hidepassed&emberchannel=canary).

It should be ok to wrap the test in a check for the existence of Ember.inject or Ember.inject.service that way the test will only get run in the versions of Ember that support that API. We can remove the check later once Ember.inject.service makes it into a release version of Ember.

@martndemus
Copy link
Contributor Author

martndemus commented Feb 25, 2015

@bmac Thanks for the pointer!

@martndemus martndemus changed the title [WIP] Register the store as a service Register the store as a service Feb 25, 2015
@bmac
Copy link
Member

bmac commented Feb 26, 2015

This looks good to me. @martndemus do you mind squashing your commits?

@tomdale would you like to review this pr?

@martndemus martndemus force-pushed the martndemus:store-as-a-service branch from 3ef9b0a to a75f883 Feb 26, 2015
@martndemus
Copy link
Contributor Author

martndemus commented Feb 26, 2015

@bmac I squashed the commits.

@bmac bmac added this to the 1.0.0-beta.16 milestone Feb 28, 2015
@slindberg
Copy link
Contributor

slindberg commented Mar 2, 2015

Yay!

It might also make sense to extend DS.Store from Ember.Service if it is available.

@bmac
Copy link
Member

bmac commented Mar 6, 2015

@martndemus I chatted with @tomdale at Ember Conf and he is on board with this change. I also like @slindberg's suggestion. Could you extend the store form Ember.Service when available?

@martndemus martndemus force-pushed the martndemus:store-as-a-service branch 2 times, most recently from 78524ff to 1739958 Mar 7, 2015
add test to see if store is registered as a service

removes a extra space

add integration test, using the store as a service

extend store from service

extend from service if service exists
@martndemus martndemus force-pushed the martndemus:store-as-a-service branch from 1739958 to a95592f Mar 7, 2015
if (!Service) {
Service = Ember.Object;
}

This comment has been minimized.

Copy link
@martndemus

martndemus Mar 7, 2015

Author Contributor

This was necessary to be backward compatible with ember-data <= 1.9.1

bmac added a commit that referenced this pull request Mar 7, 2015
Register the store as a service
@bmac bmac merged commit b1af203 into emberjs:master Mar 7, 2015
2 checks passed
2 checks passed
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bmac
Copy link
Member

bmac commented Mar 7, 2015

Thanks @martndemus!

@martndemus martndemus deleted the martndemus:store-as-a-service branch Mar 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.