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

Can we make lookupFactory public on ContainerProxyMixin? #125

Closed
workmanw opened this issue Mar 8, 2016 · 7 comments · Fixed by #150
Closed

Can we make lookupFactory public on ContainerProxyMixin? #125

workmanw opened this issue Mar 8, 2016 · 7 comments · Fixed by #150

Comments

@workmanw
Copy link
Contributor

workmanw commented Mar 8, 2016

With ember-data-model-fragments we have to rely on the private variation of _lookupFactory on the owner to lookup fragment classes, see: ember-data-model-fragments/.../ext.js.

This was part of the container API originally. And I know that was private and also we don't want to encourage users to be instantiating their own instances, but this is something we could really use for addons.

Originally filed as emberjs/ember.js#13065

@fivetanley
Copy link
Member

I tend to agree, it seems like indirection to add a lot of controllerFor type hooks when that just delegates to the container anyway.

@rwjblue
Copy link
Member

rwjblue commented Mar 8, 2016

The main reason that we didn't make lookupFactory public along with lookup was that we intend to change the way things work internally with regards to factory injections and while we realize that we will still have to support folks using the current instance._lookupFactory and container.lookupFactory we wanted to provide a better public API going forward.

The goal of that refactor is to remove the current requirement that we .extend the factory being looked up just to perform injections. Instead, we would like to make the system return an object that itself has access to the original factory (non-extended) and all the hash arguments passed to its .extend so that we can merge those hash args later when we actually call .create. (FWIW, this is roughly similar to what we do with contextual components).

Some rough pseudo code:

class FactoryWrapper {
  constructor(Factory) {
    this.Factory = Factory;
    this._extendValues = [];
  }

  extend() {
    this._extendValues.push(...arguments);
  }

  create() {
    return Factory.create(...this._extendValues, ...arguments);
  }
}

// ...snip...

  factoryFor(name) {
    let Factory = normalResolverStuffHere(name);
    let wrapper = new FactoryWrapper(Factory);

    wrapper.extend(injectionsFor(name));
    return wrapper;
  }

Obviously, there are a number of things missing in my example above, but that should help understand why things are the way they are today...

@bcardarella
Copy link
Contributor

I can say that ember-admin was using container.lookupFactory. We are now having to opt-into a private API in order to get ember-admin on latest versions of ember

@bcardarella
Copy link
Contributor

specifically, we use it to find templates: https://github.com/DockYard/ember-admin/blob/04db8d7c86be89a94eae657c3d4c8f7ff3e8e988/addon/mixins/model-records/write.js#L14

Because templates are just factories we have to use this function

@stefanpenner
Copy link
Member

until what @rwjblue lands, that is unfortunately just a reality for now.

@rwjblue
Copy link
Member

rwjblue commented May 3, 2016

I can say that ember-admin was using container.lookupFactory. We are now having to opt-into a private API in order to get ember-admin on latest versions of ember.

container.lookupFactory has always been private, but it is/was commonly used. The reason for prefixing with an underscore is exactly to ensure that developers understand that it is a private function. I definitely agree that we want a public version, and tried to lay out my thoughts in detail above.


specifically, we use it to find templates

@bcardarella - Templates can be looked up via owner.lookup just fine. In fact this is what Ember does internally (see route template lookup and component layout lookup).

@bcardarella
Copy link
Contributor

@rwjblue that worked, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants