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.

Sign up for GitHub

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

Jump to bottom

[CLEANUP beta] Remove array controller #11484

Merged
merged 2 commits into from
Jul 12, 2015
Merged

[CLEANUP beta] Remove array controller #11484

merged 2 commits into from
Jul 12, 2015

Conversation

cibernox
Copy link
Contributor

NO MERGE: Depends on #11479

Probably there is a few more tasks to do, but in different PRs:

  • Remove Ember.SortableMixin? Is private and it's not used anymore internally. Or alternatively make it public.
  • prefix with underscore beforeObserver/addBeforeObserver/removeBeforeObserver/beforeObserversFor and deprecate pubic interfaces in the Ember namespace

As a curiosity, ember is finally under 500KB again:
499KB

@cibernox cibernox mentioned this pull request Jun 16, 2015
Merged
@cibernox cibernox force-pushed the remove_array_controller branch from 1653ddd to bc71998 Compare June 18, 2015 08:06
@stefanpenner
Copy link
Member

Remove Ember.SortableMixin? Is private and it's not used anymore internally. Or alternatively make it public.

yes this should also be extracted.

Is there an addon for arraycontroller already?

@cibernox
Copy link
Contributor Author

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

@stefanpenner
Copy link
Member

@cibernox

  • this should also remove sortable mixin
  • this should move Ember.beforeObserver -> Ember._privateBeforeObserver

@cibernox
Copy link
Contributor Author

This weekend I'll extract this to the addon and finish this.

@cibernox cibernox force-pushed the remove_array_controller branch 3 times, most recently from e339e1f to d32f8ee Compare June 26, 2015 22:55
@stefanpenner
Copy link
Member

After some discussion with @tomdale regarding itemController. We suspect, itemController hooks should be left in the view layer, but only become functional when the legacy ArrayController addon is present.

This may be tests a-bit tricky, but likely they should also be extracted to the add-on.

Going forward, we will likely have to be careful to run test against the supported legacy addons, or ideally eventually support them as part of a normal test run.

stefanpenner
stefanpenner reviewed Jun 29, 2015
View reviewed changes
packages/ember-htmlbars/lib/keywords/each.js
let keyword = hash['-legacy-keyword'] && getValue(hash['-legacy-keyword']);

if (firstParam && firstParam instanceof ArrayController) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe these hooks will need to stay (as deprecated) for some reasonable period of 2.x. This will allow apps to upgrade, and for tests in the legacy addons to tap in.

Obviously requiring the ArrayController for the instanceof wont work, so we will need to revert to duckTyping of some kind. Maybe something like: firstParam.isArrayController ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added back

@stefanpenner
Copy link
Member

Remove generateController abstraction?

This may need to want until routableComponents land. Although generateController should only generate plan controllers, not object or array.

@stefanpenner
Copy link
Member

Remove ControllerMixin?

this stays around, since Ember.Controller is still needed as we did not land RoutableComponents.

@stefanpenner
Copy link
Member

@cibernox thanks for all your work here, I am super excited for this to land!

@cibernox cibernox force-pushed the remove_array_controller branch 3 times, most recently from 1d486ff to 1168c63 Compare July 4, 2015 23:12
@stefanpenner
Copy link
Member

@cibernox this looks good (except for the failing tests) Im available tomorrow and thursday to help you in anyway to get this in.

@cibernox cibernox force-pushed the remove_array_controller branch 5 times, most recently from 7579bee to 4919d19 Compare July 10, 2015 23:30
@stefanpenner
Copy link
Member

now needs a rebase

lets be sure we get the addon to production readiness shortly.

@tomdale
Copy link
Member

tomdale commented Jul 11, 2015

Is @cibernox working on extracting the addon as well?

@stefanpenner
Copy link
Member

Yes. He has it on his account Appears to be complete (to the degree discussed )

@cibernox
[CLEANUP beta] Remove ArrayController
9899dff 
Support for legacy array controller lives in the addon detected via duck
typing.

Also added inline comment where some functionality was extracted into
the addon
@cibernox cibernox force-pushed the remove_array_controller branch from 4919d19 to e9bb9bf Compare July 11, 2015 08:12
@cibernox cibernox force-pushed the remove_array_controller branch 3 times, most recently from 1be8859 to a75762e Compare July 11, 2015 18:37
@cibernox
Copy link
Contributor Author

Apparently the tests with the old jquery are consistently failing in travis, but locally they work. Is is a known issue?

@stefanpenner
Copy link
Member

Is is a known issue?

No, master appears to be green.

@cibernox cibernox force-pushed the remove_array_controller branch 2 times, most recently from 6f31f13 to deec0be Compare July 11, 2015 23:11
@cibernox
Copy link
Contributor Author

Ok, it was random. It passed after 5 tries.

@stefanpenner
Copy link
Member

sorry the second commit should likely also be BETA

@rwjblue c/d?

@cibernox cibernox force-pushed the remove_array_controller branch 12 times, most recently from 047cc25 to f829ba8 Compare July 12, 2015 14:59
@cibernox cibernox force-pushed the remove_array_controller branch from f829ba8 to 0272681 Compare July 12, 2015 18:51
rwjblue added a commit that referenced this pull request Jul 12, 2015
@rwjblue
Merge pull request #11484 from cibernox/remove_array_controller
5574f19 
[CLEANUP beta] Remove array controller
@rwjblue rwjblue merged commit 5574f19 into emberjs:master Jul 12, 2015
@rwjblue
Copy link
Member

rwjblue commented Jul 12, 2015

Thank you!

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
Labels
Needs Team Discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cibernox @stefanpenner @tomdale @rwjblue