Light Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[CLEANUP beta] Remove ObjectController#11479

Merged
stefanpenner merged 1 commit intoemberjs:masterfrom
cibernox:remove_object_controller
Jul 11, 2015
Merged

[CLEANUP beta] Remove ObjectController#11479
stefanpenner merged 1 commit intoemberjs:masterfrom
cibernox:remove_object_controller

Conversation

Copy link
Contributor

cibernox commented Jun 15, 2015

ArrayController will be removed in a different PR.

Some tests for itemController that used ObjectController could have been refactored to use a basic controller, but since it's also a deprecated functionality I just deleted them too.

Copy link
Member

stefanpenner commented Jun 16, 2015

Some tests for itemController that used ObjectController could have been refactored to use a basic controller, but since it's also a deprecated functionality I just deleted them too.

itemController is also dead, so i think those tests can likely just be removed? Although what you endedup doing also seems fine.

cibernox mentioned this pull request Jun 16, 2015
2 tasks
Copy link
Contributor Author

cibernox commented Jun 16, 2015

@stefanpenner They will be removed eventually (and actually #11484 removes more), but since it's not directly related to this (probably it's more related to the elimination of deprecated {{each}} usages) I removed the minimum amount of stuff to get the tests green.

cibernox force-pushed the remove_object_controller branch from 510d3ce to ee8a0c0 Compare June 18, 2015 08:06
Copy link
Member

stefanpenner commented Jun 18, 2015

cc @rwjblue

Copy link
Member

stefanpenner commented Jun 18, 2015

is there a legacy addon for this to live in ?

Copy link
Member

mixonic commented Jun 18, 2015

There is no addon for maintaining legacy controller APIs. You must create one if we plan to extract this functionality and the corresponding tests (or enable the functionality via a private flag).

Copy link
Contributor Author

cibernox commented Jun 18, 2015

I will create an addon this weekend, so put this on hold for now.

Just to clarify, the functionality that we intend to extract is only the class itself (ObjectController and ArrayController), but not things like it's usage within itemControllers or the automatic generation depending on what's returned form the model hook, right?

Copy link
Member

stefanpenner commented Jun 18, 2015

the functionality that we intend to extract is only the class itself (ObjectController and ArrayController)

yes

but not things like it's usage within itemControllers

i dont think so, but someone else should confirm cc @wycats / @rwjblue

the automatic generation depending on what's returned form the model hook, right?

yup not this.

Copy link
Contributor Author

cibernox commented Jun 20, 2015

Extracted addon for ObjectController and ArrayController: https://github.com/cibernox/ember-proxy-controllers

Copy link
Member

mixonic commented Jun 20, 2015

@cibernox awesome. I would like the repo to live under the Ember project- will make a place for it this afternoon when I am at a laptop.

Copy link
Member

stefanpenner commented Jun 21, 2015

Copy link
Contributor Author

cibernox commented Jun 21, 2015

I'll push to that repo it this evening

Copy link
Member

stefanpenner commented Jun 21, 2015

@cibernox emberjs/ember-legacy-controllers#1

I have left some comments for you

Copy link
Member

ebryn commented Jun 21, 2015

we need to be very careful about this extraction, the plugin is going to be used by many people and this is going to be part of their ember 2.x upgrade experience.

the functionality that we intend to extract is only the class itself (ObjectController and ArrayController), > but not things like it's usage within itemControllers or the automatic generation depending on what's returned form the model hook, right?

all existing behavior must be maintained with the plugin. all of the existing tests should still pass with it installed and it should work & feel exactly like it did before (minus the deprecation warnings)

Copy link
Member

stefanpenner commented Jun 21, 2015

@ebryn @rwjblue @cibernox it is likely also a good idea if we can run the extracted tests as part of embers own test suite, or we wont easily tell we broke something.

I realize this is all lots of work, but i suspect it will be less work in the long run.

Copy link
Member

ebryn commented Jun 21, 2015

@stefanpenner i recall talking to @wycats about how we can surface failures on canary from community projects using ember-try. i think even @rwjblue told me some work has started on that?

Copy link
Contributor Author

cibernox commented Jun 21, 2015

all existing behavior must be maintained with the plugin. all of the existing tests should still pass with it installed and it should work & feel exactly like it did before (minus the deprecation warnings)

I kind of disagree with this.

I agree that the extracted classes should work exactly as they did before, but some behaviour like the automatic generation of Object/ArrayControllers based on the returned values of the model hook has been deprecated and considered harmful for a long time. I think that is acceptable and even a good thing to force users to explicitly extend generate and extend from those controllers. If we keep that functionality and also remove deprecations we are helping to perpetuate this ad infinitum.

Copy link
Member

stefanpenner commented Jun 21, 2015

Automatic generation yes cannot continue to work, I suspect @ebryn did not take that into account. Users will need to be informed how to migrate though, as auto generated won't continue to work.

stefanpenner added the Needs Team Discussion label Jun 26, 2015
cibernox force-pushed the remove_object_controller branch 2 times, most recently from cabd779 to cf5844b Compare June 29, 2015 18:17
cibernox force-pushed the remove_object_controller branch from cf5844b to b794d76 Compare July 4, 2015 23:11
Copy link
Member

stefanpenner commented Jul 8, 2015

@cibernox i think you have addressed all the concerns in the other PR. I believe we can move forward.

As mentioned in the other PR i'll be available for you tomorrow/thursday to unblock and move to merg.

cibernox force-pushed the remove_object_controller branch 3 times, most recently from 227601f to eba2437 Compare July 10, 2015 23:25
ArrayController will be removed in a different PR
cibernox force-pushed the remove_object_controller branch from eba2437 to 56bb97e Compare July 10, 2015 23:49
Copy link
Contributor Author

cibernox commented Jul 11, 2015

I'm revisiting this. What are the blockers on the PR and the array counterpart?

stefanpenner added a commit that referenced this pull request Jul 11, 2015
[CLEANUP beta] Remove ObjectController
stefanpenner merged commit ab9d96f into emberjs:master Jul 11, 2015
cibernox deleted the remove_object_controller branch July 11, 2015 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants