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

`factoryFor` as a public API replacing `_lookupFactory` #150

Merged
merged 4 commits into from Oct 7, 2016

Conversation

Projects
None yet
10 participants
@mixonic
Member

mixonic commented Jul 1, 2016

Rendered

Based on conversation from the F2F and with preliminary review and modifications by @stefanpenner, @rwjblue.

Closes #125

@krisselden

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Jul 1, 2016

Member

Let's please use constructor instead and say it is optional, I think it is ok to register a factory to create an object literal or use Object.create().

Member

krisselden commented Jul 1, 2016

Let's please use constructor instead and say it is optional, I think it is ok to register a factory to create an object literal or use Object.create().

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jul 1, 2016

Member

Let's please use constructor instead and say it is optional, I think it is ok to register a factory to create an object literal or use Object.create().

constructor may have meaning in that position, which is why class was chosen.

Member

stefanpenner commented Jul 1, 2016

Let's please use constructor instead and say it is optional, I think it is ok to register a factory to create an object literal or use Object.create().

constructor may have meaning in that position, which is why class was chosen.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Jul 1, 2016

Member

@krisselden regarding registration- You're looking for language that it is acceptable to call owner.register('foo:baz', {create(opts) { return whatever }}), and that would work with both lookup and factoryFor? Anything with a create method is considered a valid factory?

Member

mixonic commented Jul 1, 2016

@krisselden regarding registration- You're looking for language that it is acceptable to call owner.register('foo:baz', {create(opts) { return whatever }}), and that would work with both lookup and factoryFor? Anything with a create method is considered a valid factory?

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jul 1, 2016

Member

@mixonic - That protocol (having a .create and possibly an .extend method on an object) generally works today, and I think it should be documented (though I'm unsure that documentation effort is specifically related to this RFC).

Member

rwjblue commented Jul 1, 2016

@mixonic - That protocol (having a .create and possibly an .extend method on an object) generally works today, and I think it should be documented (though I'm unsure that documentation effort is specifically related to this RFC).

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Jul 1, 2016

Member

Seem closely enough related to fold in. Today extend is required for DI to be supported, and it is indeed a meaningful change that only create will be required when this lands.

Member

mixonic commented Jul 1, 2016

Seem closely enough related to fold in. Today extend is required for DI to be supported, and it is indeed a meaningful change that only create will be required when this lands.

@martndemus

This comment has been minimized.

Show comment
Hide comment
@martndemus

martndemus Jul 1, 2016

I think the how we teach this section should also mention a migration guide for most of the cases that will break.

@rwjblue's example implementation in #125 also seems like it could be added to the RFC. His comment tells the story much better than this 280 line RFC.

Could you also add some reduced examples to the drawbacks section? The links towards projects do not always paint the picture of what is meant very clearly.

martndemus commented Jul 1, 2016

I think the how we teach this section should also mention a migration guide for most of the cases that will break.

@rwjblue's example implementation in #125 also seems like it could be added to the RFC. His comment tells the story much better than this 280 line RFC.

Could you also add some reduced examples to the drawbacks section? The links towards projects do not always paint the picture of what is meant very clearly.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Jul 2, 2016

Member

@martndemus a migration guide draft seems appropriate here.

His comment tells the story much better than this 280 line RFC.

😭 which you've asked me to add further details to :trollface: I'll add a link to his comment somewhere relevant.

Could you also add some reduced examples to the drawbacks section?

There are really not any good use-cases where you need this behavior, so any reduced example would also be pretty contrived. I will attempt to explain what the examples I've linked to are.

Member

mixonic commented Jul 2, 2016

@martndemus a migration guide draft seems appropriate here.

His comment tells the story much better than this 280 line RFC.

😭 which you've asked me to add further details to :trollface: I'll add a link to his comment somewhere relevant.

Could you also add some reduced examples to the drawbacks section?

There are really not any good use-cases where you need this behavior, so any reduced example would also be pretty contrived. I will attempt to explain what the examples I've linked to are.

@martndemus

This comment has been minimized.

Show comment
Hide comment
@martndemus

martndemus Jul 2, 2016

The more ELI5 this RFC is, the more easier it is to crowdsource docs/guides updates. 😄

martndemus commented Jul 2, 2016

The more ELI5 this RFC is, the more easier it is to crowdsource docs/guides updates. 😄

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Jul 11, 2016

Member

I've pushed a round of edits here, much improved. Please give it another look!

Member

mixonic commented Jul 11, 2016

I've pushed a round of edits here, much improved. Please give it another look!

Currently, factories registered into Ember's DI system are required to
provide an `extend` method. Removing support for extend-based DI in `_lookupFactory`
will permit factories without `extend` to be registered. Instead factories

This comment has been minimized.

@stefanpenner

stefanpenner Jul 15, 2016

Member

I agree with this change, but it is likely the second "breaking change" in this RFC. Is this a reasonable concern, or is it ok, and if so how do we migrate forwards + minimize any potential fallout. Or do we need more information/use-cases...

@stefanpenner

stefanpenner Jul 15, 2016

Member

I agree with this change, but it is likely the second "breaking change" in this RFC. Is this a reasonable concern, or is it ok, and if so how do we migrate forwards + minimize any potential fallout. Or do we need more information/use-cases...

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jul 15, 2016

Member

I am slightly confused, during what part of the rollout do we actually diverge the inheritance structure to the new simpler form?

I don't believe we can doit immedately as usages of lookupFactory('router:main').reopen(y) wont work as expected, but given some time (e.g. a deprecation cycle) people can reasonable have transitioned..

Member

stefanpenner commented Jul 15, 2016

I am slightly confused, during what part of the rollout do we actually diverge the inheritance structure to the new simpler form?

I don't believe we can doit immedately as usages of lookupFactory('router:main').reopen(y) wont work as expected, but given some time (e.g. a deprecation cycle) people can reasonable have transitioned..

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Jul 15, 2016

Member

@stefanpenner this proposal is fairly aggressive in that it suggests removal of the _lookupFactory API in Ember 2.9. See: https://github.com/mixonic/rfcs/blob/eccbd06ef6dfd4b50d1c8d4bb45ad58d110334f4/text/0000-factoryFor.md#step-2-deprecate-applicationinstance_lookupfactory-in-ember-28-remove-in-29 Ember 2.8-LTS would provide both APIs, giving LTS supporting addons/apps until Ember 2.12-LTS to make the migration.

At the same time, this RFC also proposes releasing a polyfill for factoryFor going back to at least Ember 2.4. An application (or addon) migration strategy would be to migrate to the new API via the polyfill, then remove that polyfill when support for Ember pre-2.9 is no longer a requirement.

Member

mixonic commented Jul 15, 2016

@stefanpenner this proposal is fairly aggressive in that it suggests removal of the _lookupFactory API in Ember 2.9. See: https://github.com/mixonic/rfcs/blob/eccbd06ef6dfd4b50d1c8d4bb45ad58d110334f4/text/0000-factoryFor.md#step-2-deprecate-applicationinstance_lookupfactory-in-ember-28-remove-in-29 Ember 2.8-LTS would provide both APIs, giving LTS supporting addons/apps until Ember 2.12-LTS to make the migration.

At the same time, this RFC also proposes releasing a polyfill for factoryFor going back to at least Ember 2.4. An application (or addon) migration strategy would be to migrate to the new API via the polyfill, then remove that polyfill when support for Ember pre-2.9 is no longer a requirement.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jul 15, 2016

Member

@mixonic this RFC describes the API, but does not describe when we deprecate the internal behavior (the inheritance change). Remember we cannot support both inheritance hierarchies simultaneously.

basically, at all times the following should be true:

lookupFactory(x) === factoryFor(x).class

I suspect we should do the following:

  1. deprecate _lookupFactory 2.8
  2. remove double extend after LTS that includes _lookupFactory deprecation.
  3. remove _lookupFactory entirely 3.0

I'm not sure how we can polyfil the inheritance hierarchy switch, although we can polyfil the external API's cosmetically.

Member

stefanpenner commented Jul 15, 2016

@mixonic this RFC describes the API, but does not describe when we deprecate the internal behavior (the inheritance change). Remember we cannot support both inheritance hierarchies simultaneously.

basically, at all times the following should be true:

lookupFactory(x) === factoryFor(x).class

I suspect we should do the following:

  1. deprecate _lookupFactory 2.8
  2. remove double extend after LTS that includes _lookupFactory deprecation.
  3. remove _lookupFactory entirely 3.0

I'm not sure how we can polyfil the inheritance hierarchy switch, although we can polyfil the external API's cosmetically.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jul 15, 2016

Member

In the meeting we also discussed wrapping .class in a proxy for development, a proxy which:

  • disallows all sets & obvious mutations.
  • disallows reopen reopenClass
  • allows all reads
  • allows all other operations
Member

stefanpenner commented Jul 15, 2016

In the meeting we also discussed wrapping .class in a proxy for development, a proxy which:

  • disallows all sets & obvious mutations.
  • disallows reopen reopenClass
  • allows all reads
  • allows all other operations
@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jul 15, 2016

Member

@mixonic / @rwjblue I (well actually @krisselden) have found a legit in-the-wild example of my concern raised in chat yesterday, on how we will end up breaking real code by no longer calling extend: https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/lib/views/outlet.js#L49

I remember we at-least used to do this often, but couldn't remember a current scenario.

One work-around would be to only not call extend if it is the extend we know to be the one on an ember object... Im not sure there is a downside yet, but will likely require more thought.

Member

stefanpenner commented Jul 15, 2016

@mixonic / @rwjblue I (well actually @krisselden) have found a legit in-the-wild example of my concern raised in chat yesterday, on how we will end up breaking real code by no longer calling extend: https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/lib/views/outlet.js#L49

I remember we at-least used to do this often, but couldn't remember a current scenario.

One work-around would be to only not call extend if it is the extend we know to be the one on an ember object... Im not sure there is a downside yet, but will likely require more thought.

@krisselden

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Jul 15, 2016

Member

I would not be surprised if there is not addons that use extend to return a custom factory

Member

krisselden commented Jul 15, 2016

I would not be surprised if there is not addons that use extend to return a custom factory

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jul 15, 2016

Member

I would not be surprised if there is not addons that use extend to return a custom factory

Yes, i believe we can detect those cases and do the old thing.

Member

stefanpenner commented Jul 15, 2016

I would not be surprised if there is not addons that use extend to return a custom factory

Yes, i believe we can detect those cases and do the old thing.

@sukima

This comment has been minimized.

Show comment
Hide comment
@sukima

sukima Aug 9, 2016

To help this RFC along, our team is desperately trying to migrate from 1.13. One of the hardest blockers of this is our app design relied heavily on JS only imports and window/Ember globals. Newer implementations of these features use services now which are unavailable without a container. To help this migration we would love to get our dependency injections set for our Ember.Object based models so that when we do eventually convert to ember-data the services we use would already be available.

The downsides are:

  • The docs show _lookupFactory as private. This makes our team members nervous.
  • The method starts with an underscore which makes our team doubly nervous.

Although _lookupFactory is a de facto standard and used among addon authors in the wild it is still an up hill battle in some teams to convince them that a private method that starts with an underscore is technically safe.

This RFC would offer a sanctioned way to create objects via a factory with dependency injection where the usual Ember.Object.create would not otherwise. The examples in this RFC are an exact match for what we need in our app to move it forward. For now we will use _lookupFactory but would really love to see this RFC move into a recognized implementation and a polyfill to get older apps there.

ember-data is great that it can do this stuff automagically but it sure is nice to be able to do these tricks on our own. As an addon author it opens up a large opportunity for some very powerful features. I hope this testimony of one possible use case in a production app helps adds some context to this RFC.

sukima commented Aug 9, 2016

To help this RFC along, our team is desperately trying to migrate from 1.13. One of the hardest blockers of this is our app design relied heavily on JS only imports and window/Ember globals. Newer implementations of these features use services now which are unavailable without a container. To help this migration we would love to get our dependency injections set for our Ember.Object based models so that when we do eventually convert to ember-data the services we use would already be available.

The downsides are:

  • The docs show _lookupFactory as private. This makes our team members nervous.
  • The method starts with an underscore which makes our team doubly nervous.

Although _lookupFactory is a de facto standard and used among addon authors in the wild it is still an up hill battle in some teams to convince them that a private method that starts with an underscore is technically safe.

This RFC would offer a sanctioned way to create objects via a factory with dependency injection where the usual Ember.Object.create would not otherwise. The examples in this RFC are an exact match for what we need in our app to move it forward. For now we will use _lookupFactory but would really love to see this RFC move into a recognized implementation and a polyfill to get older apps there.

ember-data is great that it can do this stuff automagically but it sure is nice to be able to do these tricks on our own. As an addon author it opens up a large opportunity for some very powerful features. I hope this testimony of one possible use case in a production app helps adds some context to this RFC.

@sukima

This comment has been minimized.

Show comment
Hide comment
@sukima

sukima Aug 9, 2016

For what it's worth here is how we worked with this (with out the ember-getowner-polyfill):

import Ember from 'ember';

const { isPresent, Service, deprecate } = Ember;

function getOwner(ctx) {
  deprecate(
    'Ember.getOwner() is available now; please cleanup references to this.container',
    isPresent(Ember.getOwner),
    {id: 'container-lookup'}
  );
  return isPresent(Ember.getOwner) ? Ember.getOwner(ctx) : ctx.container;
}

export default Service.extend({
  createRecord(modelName, payload) {
    const owner = getOwner(this);
    const lookupFactory = owner._lookupFactory || owner.lookupFactory;
    const ModelFactory = lookupFactory.call(owner, `model:${modelName}`);
    return ModelFactory.create(payload);
  }
});

sukima commented Aug 9, 2016

For what it's worth here is how we worked with this (with out the ember-getowner-polyfill):

import Ember from 'ember';

const { isPresent, Service, deprecate } = Ember;

function getOwner(ctx) {
  deprecate(
    'Ember.getOwner() is available now; please cleanup references to this.container',
    isPresent(Ember.getOwner),
    {id: 'container-lookup'}
  );
  return isPresent(Ember.getOwner) ? Ember.getOwner(ctx) : ctx.container;
}

export default Service.extend({
  createRecord(modelName, payload) {
    const owner = getOwner(this);
    const lookupFactory = owner._lookupFactory || owner.lookupFactory;
    const ModelFactory = lookupFactory.call(owner, `model:${modelName}`);
    return ModelFactory.create(payload);
  }
});

mixonic added some commits Jul 1, 2016

Change up the migration approach w/ factoryFor
The prior approach was too fast- this edit lays out a less aggressive
and more iterative approach.
@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Sep 16, 2016

such as instanceof checks

stefanpenner commented on text/0000-factoryFor.md in 1cb5b59 Sep 16, 2016

such as instanceof checks

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Sep 18, 2016

Member

This RFC is basically in FCP pending the process item of a tweet from https://twitter.com/emberjs and tag. If you've any remaining concerns you believe are missed here, please let me know!

Member

mixonic commented Sep 18, 2016

This RFC is basically in FCP pending the process item of a tweet from https://twitter.com/emberjs and tag. If you've any remaining concerns you believe are missed here, please let me know!

@pixelhandler

This comment has been minimized.

Show comment
Hide comment
@pixelhandler

pixelhandler Sep 26, 2016

@mixonic I use _lookupFactory in my data persistence addon. Today I read over the RFC and am very pleased with the thought and depth put into this RFC.

As an addon developer I think it will be straight forward to make the upgrade to factoryFor and at the same time continue support _lookupFactory as well. (the build for the addon tests v1.13 though canary).

👍

pixelhandler commented Sep 26, 2016

@mixonic I use _lookupFactory in my data persistence addon. Today I read over the RFC and am very pleased with the thought and depth put into this RFC.

As an addon developer I think it will be straight forward to make the upgrade to factoryFor and at the same time continue support _lookupFactory as well. (the build for the addon tests v1.13 though canary).

👍

@chadhietala

This comment has been minimized.

Show comment
Hide comment
@chadhietala

chadhietala Sep 28, 2016

Member

I would like to propose we don't inject the deprecated container into the instances returned from factoryFor and we mitigate that to the deprecated _lookupFactory path.

Member

chadhietala commented Sep 28, 2016

I would like to propose we don't inject the deprecated container into the instances returned from factoryFor and we mitigate that to the deprecated _lookupFactory path.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Sep 28, 2016

Member

@locks thats for the tagging!

@chadhietala Can you say more? There is no "deprecated container" described here.

Member

mixonic commented Sep 28, 2016

@locks thats for the tagging!

@chadhietala Can you say more? There is no "deprecated container" described here.

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Sep 30, 2016

Member

Note: To make the Final Comment Period official, we need to announce it here and on Twitter, so we're restarting the period now.


This RFC is now entering a "final comment period". This period is intended to focus the community's attention for a final round of consideration and feedback before an RFC is merged.

Please carefully consider the implications of this RFC and how it will affect your Ember projects. Now would be a good time to re-raise any concerns that you feel haven't been fully addressed.

If no significant blockers are raised by October 7, we plan to merge this RFC and make plans for its implementation. :shipit:

Member

dgeb commented Sep 30, 2016

Note: To make the Final Comment Period official, we need to announce it here and on Twitter, so we're restarting the period now.


This RFC is now entering a "final comment period". This period is intended to focus the community's attention for a final round of consideration and feedback before an RFC is merged.

Please carefully consider the implications of this RFC and how it will affect your Ember projects. Now would be a good time to re-raise any concerns that you feel haven't been fully addressed.

If no significant blockers are raised by October 7, we plan to merge this RFC and make plans for its implementation. :shipit:

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Oct 7, 2016

Member

Looks like no additional comments came through during the FCP process. Lets do it!!!

Member

rwjblue commented Oct 7, 2016

Looks like no additional comments came through during the FCP process. Lets do it!!!

@rwjblue rwjblue merged commit b3aea53 into emberjs:master Oct 7, 2016

@pixelhandler

This comment has been minimized.

Show comment
Hide comment
@pixelhandler

pixelhandler Oct 7, 2016

@rwjblue should the "Final Comment Period" label be removed?

pixelhandler commented Oct 7, 2016

@rwjblue should the "Final Comment Period" label be removed?

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Oct 7, 2016

Member

@pixelhandler good call - done.

Member

dgeb commented Oct 7, 2016

@pixelhandler good call - done.

@chadhietala

This comment has been minimized.

Show comment
Hide comment
@chadhietala

chadhietala Oct 7, 2016

Member

See emberjs/ember.js#14360 for the WIP implementation.

Member

chadhietala commented Oct 7, 2016

See emberjs/ember.js#14360 for the WIP implementation.

@mixonic mixonic deleted the mixonic:factoryFor branch Oct 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment