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

Internals should prefer generic array methods over prototype_extensions #10899

Closed
wants to merge 12 commits into from

Conversation

stefanpenner
Copy link
Member

  • addArrayObserver
  • removeArrayObserver
  • objectAt
  • replace
  • remoteAt
  • insertAt
  • pushObject
  • pushObjects
  • re-org public api
  • etc ...
  • docs
  • all the tests

The goal here is merely to decouple the internals from the the observable based prototype extensions.
This will hopefully give us the flexibility to make a future call re: prototype extensions in general. This also eases the pain of using ember with prototype extensions disabled.

All this is accomplished without breaking semver in anyway.

More todo:

  • Merge Enumerable + NativeArray
  • Merge MutableEnumerable + MutableArray
  • Deprecate Enumerable as [bugfix release]
  • Deprecate MutableEnumerable as [bugfix release]


export function removeAt(content, idx, length = 1) {
if (content.removeAt) { return content.removeAt(idx, length); }
return content.splice(idx, length);
Copy link
Contributor

Choose a reason for hiding this comment

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

what sends the event here?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah good catch. this needs to use the replace helper

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @thoov

Copy link
Member

Choose a reason for hiding this comment

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

Taking a look at this

@krisselden
Copy link
Contributor

So, this doesn't quite address the issue of Ember.A() and MutableArray replace

The standalone method of Ember.replace() should fire the event on a plain array like Ember.set() would do on an object.

@rwjblue
Copy link
Member

rwjblue commented Aug 2, 2015

Just catching up to speed on this one. Looking good so far!

@stefanpenner - Is this still on your radar? Is there any way to chunk this up (to avoid forever horrible rebase land of large PR's)? I'm mostly wondering if we can land what you have here and open a meta issue to continue...

@stefanpenner
Copy link
Member Author

Is this still on your radar?

yup

Is there any way to chunk this up (to avoid forever horrible rebase land of large PR's)?

its kinda tricky, but possible. But i'll deal with the rebases and cleanup as when i start on this again.

Lets leave this open so i don't forget.

@rwjblue
Copy link
Member

rwjblue commented Aug 3, 2015

Lets leave this open so i don't forget.

Sounds good, just was doing a bit of PR triage/cleanup over the weekend..

@stefanpenner
Copy link
Member Author

Sounds good, just was doing a bit of PR triage/cleanup over the weekend..

thanks

rwjblue pushed a commit that referenced this pull request Feb 21, 2016
Do not use prototype extension for `objectAt`.

Related to #9269 and based on the work of @stefanpenner in #10899.

If this is acceptable, will continue addressing one extension at a time for quicker merging.

(cherry picked from commit cd0d186)
rwjblue pushed a commit that referenced this pull request Feb 21, 2016
Do not use prototype extensions for `addArrayObserver` and `removeArrayObserver`.

Related to #9269 and #10899.

(cherry picked from commit c77eb92)
@pixelhandler
Copy link
Contributor

@stefanpenner is this PR inactive? Is the a path forward to continue this effort aside from this branch?

@Serabe
Copy link
Member

Serabe commented Apr 16, 2017

@stefanpenner is this still active? Has it been superseded somehow?

@stefanpenner
Copy link
Member Author

@Serabe it hasn't been superseded, and would love a champion

@Serabe
Copy link
Member

Serabe commented Apr 17, 2017

@stefanpenner I might have time, so we can chat about it.

@Serabe
Copy link
Member

Serabe commented Jul 13, 2017

Closing this in favor of using an Epic #15501

Thank you!

@Serabe Serabe closed this Jul 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants