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

Fix infinity model container so services can be injected on it #338

Merged
merged 1 commit into from
Nov 26, 2018
Merged

Fix infinity model container so services can be injected on it #338

merged 1 commit into from
Nov 26, 2018

Conversation

mossymaker
Copy link

Injecting services on the extended infinity model was failing because the instance is created without a container. This seems like the right fix?

With the 1.0 API changes, infinityModelLoaded and infinityModelUpdated no longer have the context they did as members of the route. Now, an intermediary (e.g. a service) appears to be required to effect a change in the context of the route or controller, e.g.:

import InfinityModel from 'ember-infinity/lib/infinity-model';

export default InfinityModel.extend({
  infinityModelUpdated({ lastPageLoaded, totalPages }) {
    if (totalPages > lastPageLoaded) {
      // Hide footer until all records are loaded
      set(this, 'memoryStorage.hasActiveInfiniteScroll', true);
    }
  }
});
{{! app/templates/application.hbs}}
{{#unless memoryStorage.hasActiveInfiniteScroll}}
  {{footer-stuff}}
{{/unless}}

@@ -413,7 +415,7 @@ export default Service.extend({
@param {EmberInfinity.InfinityModel} infinityModel
*/
_notifyInfinityModelUpdated(queryObject, infinityModel) {
const totalPages = get(this, '_totalPages');
const totalPages = get(infinityModel, '_totalPages');

Choose a reason for hiding this comment

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

Not positive how this is connected to other changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated but seems like an oversight. Nice catch!

Copy link
Author

Choose a reason for hiding this comment

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

Unrelated...I can pull it into another commit/PR if you prefer.

@@ -227,6 +228,7 @@ export default Service.extend({
}

let initParams = {
container: getOwner(this),

Choose a reason for hiding this comment

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

This seems like a good idea...but not sure what implications this would have, from a memory- or lifecycle-related perspective.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@callovarne What version of ember-infinity are you on? I had thought this was fixed a while back. See the below README link.

https://github.com/ember-infinity/ember-infinity#extending-infinitymodel

Copy link
Author

@mossymaker mossymaker Nov 12, 2018

Choose a reason for hiding this comment

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

I'm working off master with 1.2.6. Without giving the container, Ember reports:

Assertion Failed: Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container.

I suspect in pre-1.0, the container was set on the route when it was instantiated by Ember. Since InfinityModel isn't, it seems as though that needs to be done here.

@snewcomer
Copy link
Collaborator

@callovarne I generally approve of this PR! Lmk if you have time to add a test.

@mossymaker
Copy link
Author

@snewcomer: updated with test.

Copy link
Collaborator

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

This looks like a great change!

@snewcomer snewcomer merged commit b68eb2c into adopted-ember-addons:master Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants