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

[WIP] [BUGFIX beta] Enable pushObject as a generic array method #15652

Conversation

alvincrespo
Copy link
Contributor

Tackles #15501 pushObject task.

Changes

  • Creates an exportable pushObject method in packages/ember-runtime/lib/mixins/array.js
  • Updates the following files to use the new exported pushObject
    • packages/ember-extension-support/tests/data_adapter_test.js

@alvincrespo alvincrespo changed the title [BUGFIX beta] Enable pushObject as a generic array method [WIP BUGFIX beta] Enable pushObject as a generic array method Sep 10, 2017
@alvincrespo alvincrespo changed the title [WIP BUGFIX beta] Enable pushObject as a generic array method [WIP] [BUGFIX beta] Enable pushObject as a generic array method Sep 10, 2017
@alvincrespo
Copy link
Contributor Author

@Serabe @locks @rwjblue @stefanpenner I'm not entirely sure if what I submitted here is the goal for #15501, but I thought I'd give it a shot. Let me know if this is a good starting point and headed in the right direction. Thanks in advance! 🍻

@@ -60,6 +60,10 @@ export function objectAt(content, idx) {
return typeof content.objectAt === 'function' ? content.objectAt(idx) : content[idx];
}

export function pushObject(arr, obj) {
return typeof arr.pushObject === 'function' ? arr.pushObject(obj) : arr.push(obj);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if we should default to Array#push for this or if we should warn/error out here to let the developer know the prototype extension isn't on.

Copy link
Member

Choose a reason for hiding this comment

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

to see this complete, pushObject must be implemented off of arr so that it can be used regardless of prototype extensions or not.

@@ -60,6 +60,10 @@ export function objectAt(content, idx) {
return typeof content.objectAt === 'function' ? content.objectAt(idx) : content[idx];
}

export function pushObject(arr, obj) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was basing this on the work done here: 924bf0e - not sure if thats a good starting point. Let me know.

@alvincrespo
Copy link
Contributor Author

Hey everyone! Just curious what your thoughts are on the work here thus far? Thanks in advance.

@@ -60,6 +60,10 @@ export function objectAt(content, idx) {
return typeof content.objectAt === 'function' ? content.objectAt(idx) : content[idx];
}

export function pushObject(arr, obj) {
return typeof arr.pushObject === 'function' ? arr.pushObject(obj) : arr.push(obj);
Copy link
Member

Choose a reason for hiding this comment

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

to see this complete, pushObject must be implemented off of arr so that it can be used regardless of prototype extensions or not.

@Serabe
Copy link
Member

Serabe commented Dec 22, 2017

@alvincrespo ping

1 similar comment
@Serabe
Copy link
Member

Serabe commented Jan 5, 2018

@alvincrespo ping

@locks
Copy link
Contributor

locks commented Jan 15, 2020

@alvincrespo are you interested in finishing this work?

@alvincrespo
Copy link
Contributor Author

@Serabe @locks Apologies - I won't be able to pick this up.

I haven't used Ember in a few years now. I need to check out Octane for sure! Hopefully, sometime soon.

@locks
Copy link
Contributor

locks commented Jan 15, 2020

Hope to you see you back some time!
I'll close this for now as the codebase has seen quite a bit of change. Thank you for your efforts!

@locks locks closed this Jan 15, 2020
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.

4 participants