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

filterBy needs .@each when composed to see updates #407

Open
jlami opened this issue Sep 20, 2017 · 7 comments · May be fixed by kellyselden/ember-macro-helpers#149
Open

filterBy needs .@each when composed to see updates #407

jlami opened this issue Sep 20, 2017 · 7 comments · May be fixed by kellyselden/ember-macro-helpers#149

Comments

@jlami
Copy link

jlami commented Sep 20, 2017

It seems that without a .@each.<keytofilterby> the result will not update on changes to the key.

I have a reproduction of the bug here: https://ember-twiddle.com/d6a67b62d1a193fbbdaa67fbd5092d14?openFiles=controllers.application.js%2C

I don't really know how to work with ember-macro-test-helpers so can't really build a failing test case. But hopefully this twiddle be enough.

@kellyselden
Copy link
Owner

I verified the bug. Not sure on the fix yet. I think it's related to this line https://github.com/kellyselden/ember-macro-helpers/blob/v0.17.0/addon/flatten-keys.js#L18 not array-aware.

@Techn1x
Copy link

Techn1x commented Dec 18, 2017

Can confirm, hit this issue too today.

My use case is

alertConditions: DS.hasMany('alertCondition'),
alertConditionsFiltered: filterBy('alertConditions',raw('isDeleted'),false),	// Filter out any to-be-deleted

When I mark an alertCondition with isDeleted true, nothing updates. When it's set back to false it does update.

Looks like there's some work towards solving this, but thought I'd add my comment in case its helpful.

@lolmaus
Copy link
Contributor

lolmaus commented Dec 21, 2017

When I do filterBy("reviewers", "voted", true) with ember-awesome-macros, then the raw value of $E.voters._dependentKeys[0] is `"[email protected]".

But when I do voters: filterBy("reviewers", raw("voted"), true) with ember-awesome-macros, then the raw value of $E.voters._dependentKeys[0] is `"reviewers".

I even tried voters: filterBy("[email protected]", raw("voted"), true) and it still didn't work.


I tried the ember-macro-helpers#composing-class-computed branch from kellyselden/ember-macro-helpers#149 and it did not help.


I've been slowly converting my app to ember-awesome-macros (because I'm a huge fan), but this bug burns badly. I wonder how many regressions my app has already?

@kellyselden, which other macros may be affected by this?

Will you accept a PR that adds checks for .@each. inside _dependentKeys[0] to tests of every array macro?

@kellyselden
Copy link
Owner

I'm not sure how wide-spread this issue is.

I think expanding on the failing test in #149 to cover the cases I missed would be a good start.

@lolmaus
Copy link
Contributor

lolmaus commented Dec 22, 2017

I've discovered that my issue is a separate bug: array macros fail with Array-like objects such as DS.ManyArray.

I've reported it in #423, covered with a failing test in 1e55173 and proposed a fix in #424.

@Techn1x
Copy link

Techn1x commented Aug 16, 2018

So I just noticed that above in my comment, I had a mistake, I should have been using raw(false) - not sure if this matters or not.

Whether or not that is what my code was too at the time, I don't know since it was over 8 months ago.

For what it's worth, I've just updated from 0.41 to 3.0.0 and tested filterBy again, and it seems to work now, using the following code;

//app/models/alert.js
alertConditions: DS.hasMany('alertCondition'),
alertConditionsFiltered: filterBy('alertConditions',raw('isDeleted'),raw(false)),	// Filter out any to-be-deleted
{{!-- After saving an alert with several conditions, then deleting a condition locally before saving again, marks the condition as 'isDeleted' --}}
{{!-- This template should now show the correct number each time a condition is deleted --}}
{{#if (gt alert.alertConditionsFiltered.length 0)}}
  {{!-- previously, this number wouldn't go down --}}
  Number of alert conditions: {{alert.alertConditionsFiltered.length}}
{{else}}
  No conditions specified
{{/if}}

But as I said, my use case / testing could have been wrong to begin with. If someone else could confirm this is fixed that would be great.

@lolmaus
Copy link
Contributor

lolmaus commented Sep 24, 2019

This bug is now two years old! 🎂

@kellyselden Are you going to address this?

ember-awesome-macros has been my absolute favorite Ember addon for years, but issues like this prevent me from recommending it for use in fresh projects.

PS I would appreciate a statement regarding the addon's current status and future — in terms of maintenance, development and alignment with Ember's new course.

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 a pull request may close this issue.

4 participants