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

[BUGFIX release] fix regression with computed filter/map/sort #15855

Merged
merged 3 commits into from
Nov 28, 2017
Merged

[BUGFIX release] fix regression with computed filter/map/sort #15855

merged 3 commits into from
Nov 28, 2017

Conversation

bekzod
Copy link
Contributor

@bekzod bekzod commented Nov 14, 2017 •
edited
Loading

filter/map/sort would not expand items.@each.{prop1,prop2} therefore would not properly work

@bekzod
Copy link
Contributor Author

bekzod commented Nov 14, 2017

fixes #15843

rwjblue
rwjblue requested changes Nov 14, 2017
View reviewed changes
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

This regression was introduced in #15550 which changed both the reduceMacro, multiArrayMacro,and arrayMacro ( the one fixed in this PR) invocations to directly pass dependentKeys in options (and therefore avoiding expand properties).

Can you add a test for the same regression that uses multiArrayMacro (Ember.computed.uniq for example) and reduceMacro (Ember.computed.sum for example) using brace expansion, and update those implementations also?

packages/ember-runtime/lib/computed/reduce_computed_macros.js Outdated
}, { dependentKeys: [ dependentKey ], readOnly: true });
}, { readOnly: true });

cp.property(dependentKey);
Copy link
Member

Choose a reason for hiding this comment

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

👍 the fix is good, but I wanted to comment here to explain why this is different.

The ComputedProperty constructor does not run the provided dependent keys through expandProperties (perhaps it should, but that is another issue altogether). So moving this out from the constructor options into cp.property forces it to run expandProperties.

Copy link
Contributor Author

@bekzod bekzod Nov 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

it is intentional that constructor argument does not run through expandProperties to avoid extra work (so it expects expanded properties already) because I made dependent keys passable through constructor :P actually

@bekzod
Copy link
Contributor Author

bekzod commented Nov 14, 2017 •
edited
Loading

I think rest of computed properties are not affected by regression, because computed properties that use reduceMacro and multiArrayMacro just expect array property name, I can add assertion for passing expandable properties to computed properties that use reduceMacro/multiArrayMacro (for sum, max, min) if it is necessary

@rwjblue
Copy link
Member

rwjblue commented Nov 14, 2017

I can add assertion for passing expandable properties to computed properties that use reduceMacro/multiArrayMacro (for sum, max, min) if it is necessary

Seems good to me. Can you add that assertion in a separate commit (as a [BUGFIX beta] instead of release)?

rwjblue
rwjblue reviewed Nov 14, 2017
View reviewed changes
packages/ember-runtime/lib/computed/reduce_computed_macros.js Outdated
function reduceMacro(dependentKey, callback, initialValue) {
function reduceMacro(dependentKey, callback, initialValue, name) {
assert(
`Dependent key passed to Ember.computed.${name}() shouldn't contain brace expading pattern.`,
Copy link
Member

Choose a reason for hiding this comment

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

typo s/expading/expanding/

rwjblue
rwjblue reviewed Nov 14, 2017
View reviewed changes
packages/ember-runtime/lib/computed/reduce_computed_macros.js Outdated
function multiArrayMacro(_dependentKeys, callback) {
function multiArrayMacro(_dependentKeys, callback, name) {
assert(
`Dependents keys passed to Ember.computed.${name}() shouldn't contain brace expading pattern.`,
Copy link
Member

Choose a reason for hiding this comment

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

same as above

rwjblue
rwjblue requested changes Nov 14, 2017
View reviewed changes
packages/ember-runtime/lib/computed/reduce_computed_macros.js Outdated
@@ -17,8 +17,12 @@ import compare from '../compare';
import { isArray } from '../utils';
import { A as emberA } from '../system/native_array';

function reduceMacro(dependentKey, callback, initialValue, name) {
assert(
`Dependent key passed to Ember.computed.${name}() shouldn't contain brace expading pattern.`,
Copy link
Member

Choose a reason for hiding this comment

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

s/expading/expanding/

packages/ember-runtime/lib/computed/reduce_computed_macros.js Outdated
function multiArrayMacro(_dependentKeys, callback) {
function multiArrayMacro(_dependentKeys, callback, name) {
assert(
`Dependent keys passed to Ember.computed.${name}() shouldn't contain brace expading pattern.`,
Copy link
Member

Choose a reason for hiding this comment

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

s/exading/expanding/

@bekzod bekzod force-pushed the filter-computed-expand branch 4 times, most recently from 837adae to 2b557dc Compare November 14, 2017 20:21
@urbany
Copy link

urbany commented Nov 21, 2017

Hi, will this make it to 2.17?

@rwjblue
Copy link
Member

rwjblue commented Nov 28, 2017

I believe so (and ultimately backported to 2.16 also)...

@rwjblue rwjblue merged commit 87efb0a into emberjs:master Nov 28, 2017
@Serabe Serabe mentioned this pull request Nov 30, 2017
Closed
@karthiicksiva karthiicksiva mentioned this pull request Dec 5, 2017
Closed
@bekzod bekzod deleted the filter-computed-expand branch December 20, 2017 11:21
@mmun mmun mentioned this pull request Feb 21, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bekzod @rwjblue @urbany @ilucin