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

[BUGFIX beta] Allow ArrayProxy#pushObjects to accept ArrayProxy again #16853

Merged
merged 1 commit into from Aug 1, 2018
Merged

[BUGFIX beta] Allow ArrayProxy#pushObjects to accept ArrayProxy again #16853

merged 1 commit into from Aug 1, 2018

Conversation

nlfurniss
Copy link
Contributor

@nlfurniss nlfurniss commented Jul 31, 2018

This fixes #16621

@rwjblue and @hjdivad proposed the following solution:

  • Drop the assertion in pushObjects entirely
  • docs: Implementations have to assert their arguments in replace (which is the method implementations of MutableArray are expected to implement)

I'm don't know enough about Ember to know what #2 means, but would be happy to implement it once I understand what needs to be added to the docs.

@rwjblue
Copy link
Member

rwjblue commented Jul 31, 2018

Looks like the CI failures are related to the changes here...

@nlfurniss
Copy link
Contributor Author

Weird, wonder why that didn't fail when i ran the tests locally...

What should I do with this failing test? Remove it?

@rwjblue
Copy link
Member

rwjblue commented Jul 31, 2018

Only the production build tests were failing because we are using assert.throws, however now we are no longer throw new Error() we rely on the replace implementation to do it and it does an Ember.assert (which is stripped in production builds).

tldr; s/assert.throws/expectAssertion/ in that file

@nlfurniss
Copy link
Contributor Author

Added docs and travis ci passes 🎉

@hjdivad
Copy link
Member

hjdivad commented Jul 31, 2018

docs: Implementations have to assert their arguments in replace (which is the method implementations of MutableArray are expected to implement)

I'm don't know enough about Ember to know what #2 means, but would be happy to implement it once I understand what needs to be added to the docs.

It means updating the docs in MutableArray#replace to make clear that part of the MutableArray contract is that any type checking done in insertion is done specifically in replace.

As a bonus ArrayProxy#replace should probably refer to the MutableArray#replace docs.

@nlfurniss
Copy link
Contributor Author

nlfurniss commented Jul 31, 2018

Travis failed due to disconnect, but last 2 runs were clean. Would re-run but dont have permission

@hjdivad
Copy link
Member

hjdivad commented Aug 1, 2018

i've restarted the job

@rwjblue rwjblue merged commit 24ce7de into emberjs:master Aug 1, 2018
@nlfurniss nlfurniss deleted the fix-array-push branch August 1, 2018 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

regression 3.0: ArrayProxy#pushObjects doesn't accept ArrayProxy
3 participants