Skip to content
This repository has been archived by the owner on Apr 18, 2023. It is now read-only.

Use owner.factoryFor is present #431

Merged
merged 1 commit into from Feb 13, 2017
Merged

Use owner.factoryFor is present #431

merged 1 commit into from Feb 13, 2017

Conversation

cibernox
Copy link
Collaborator

@cibernox cibernox commented Jan 24, 2017 •
edited

This removes deprecations and should have a very positive perf outcome in Ember 2.12

@cibernox cibernox force-pushed the use_factory_for branch 2 times, most recently from ac9dccd to 535c692 Compare January 24, 2017 11:30
@workmanw
Copy link
Contributor

Just looking at these changes, it's not obvious to me where the performance improvement comes from. I was wondering if you could elaborate on that or maybe point me toward something I could read.

@cibernox
Copy link
Collaborator Author

The factoryFor method seems to be way faster, according with the release notes of 2.12.
And even if it wasn't, the other method is deprecated.

@rwjblue
Copy link
Contributor

rwjblue commented Jan 24, 2017

The factory for RFC outlines the perf benefits. The short version is that Ember no longer has to .extend every factory a second time (which we have done for a long time to ensure it gets the proper injections).

@workmanw
Copy link
Contributor

Oh awesome! Thank you @cibernox @rwjblue. I read the PR, but it wasn't clear to me there. I will definitely go reread the RFC. 👍

bwbuchanan
bwbuchanan reviewed Jan 24, 2017
View reviewed changes
Copy link

@bwbuchanan bwbuchanan left a comment •
edited

Choose a reason for hiding this comment

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

The call to owner._lookupFactory in getFlattenedTranslations() also needs to be updated.

@cibernox
Copy link
Collaborator Author

@bwbuchanan Added

bwbuchanan
bwbuchanan suggested changes Jan 24, 2017
View reviewed changes
addon/utils/locale.js Outdated
let translations;
if (owner.factoryFor) {
let maybeTranslations = owner.factoryFor(`locale:${id}/translations`);
translations = translations && translations.class;
Copy link

Choose a reason for hiding this comment

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

Should be translations = maybeTranslations && maybeTranslations.class;

@bwbuchanan
Copy link

Looks good. My app's tests are green.

@cibernox
Copy link
Collaborator Author

cibernox commented Jan 24, 2017 •
edited

Actually mine had so many deprecations that tests broke because travis cannot handle a log that big. Gives how much this addon used this feature that is why I hope the perf boost will nice.

@workmanw workmanw mentioned this pull request Jan 24, 2017
Closed
3 tasks
rwjblue
rwjblue reviewed Jan 26, 2017
View reviewed changes
Copy link
Contributor

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

I flagged a couple examples, but generally speaking we need to just use the factory object that is returned by factoryFor and not call .class on it.

Happy to take another look once that is updated...

addon/services/i18n.js Outdated
let owner = getOwner(this);
let ENV;
if (owner.factoryFor) {
ENV = owner.factoryFor('config:environment').class;
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we should return the object returned by factoryFor (not the .class property)

addon/utils/add-translations.js Outdated
let existingTranslations;
if (owner.factoryFor) {
let maybeTranslations = owner.factoryFor(key);
existingTranslations = maybeTranslations && maybeTranslations.class;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove .class

@cibernox
Copy link
Collaborator Author

cibernox commented Jan 26, 2017 •
edited

Feedback applied.

This was referenced Jan 26, 2017
Merged
Merged
@cibernox cibernox force-pushed the use_factory_for branch 2 times, most recently from cd70ed1 to b1cf707 Compare January 30, 2017 11:26
@cibernox
Use owner.factoryFor
fe39fa2 
This removes deprecations and should have a very positive perf outcome
@cibernox cibernox merged commit dd5157d into jamesarosen:master Feb 13, 2017
@cibernox cibernox deleted the use_factory_for branch February 13, 2017 20:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rwjblue rwjblue rwjblue left review comments

@bwbuchanan bwbuchanan bwbuchanan approved these changes

@locks locks locks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cibernox @workmanw @rwjblue @bwbuchanan @locks