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

Fix sort-by to accept Ember Data relationship #367

Merged
merged 1 commit into from
Jun 4, 2020

Conversation

jrjohnson
Copy link
Contributor

@jrjohnson jrjohnson commented Jun 3, 2020

The refactor of sort-by in #362 by @snewcomer no longer accepts a resolved Ember Data hasMany relationship as this line throws array is not iterable.

@snewcomer
Copy link
Contributor

Thanks for the PR! This seems like it would be a problem for all the helpers, not just sort-by. I'm not exactly sure of a solution off the top of my head but for awaiting the promise before passing into any of the ember-composable-helpers...

@jrjohnson
Copy link
Contributor Author

I'm not sure either, it works everywhere else at this point (and worked in sort-by in 4.0.0). I didn't want to jam up this PR with more tests, but I'll add those tests for all the remaining helpers once this is resolved here to prevent another regression.

Our particular use case is to pass an ember data relationship in using the await helper like

{{#each (sort-by "lastName" @course.directors) as |user|}}
{{/each}}

So the promise is actually resolved, but it's passing in a resolved promise and not an array. I couldn't come up with an elegant way to check if something was an already resolved promise and pull the value, but that is probably what is needed (as the helpers, in my opinion, shouldn't be async).

@snewcomer
Copy link
Contributor

Feel free to add some more tests to this PR! I was looking at filter and was curious how that would work wrapped in a Promise as well.

EmberArray isn't iterable and .toArray needs to be called on it to
return the original array value.
@jrjohnson jrjohnson force-pushed the fulfilled-promise-to-sort-by branch from 91b3934 to 0b0d2d4 Compare June 4, 2020 06:07
@jrjohnson
Copy link
Contributor Author

You're right! Sorry for the red-herring. The issue isn't the promise, but the EmberArray that is resolved from a hasMany Ember Data relationship. Which points to a fairly simple fix. I also added a couple tests in other places while I was experimenting and left those in for posterity.

@jrjohnson jrjohnson changed the title Add failing test for promise as value to sort-by Fix sort-by to accept Ember Data relationship Jun 4, 2020
@GavinJoyce
Copy link
Contributor

👍 nice one @jrjohnson, I tried to upgrade ember-composable-helpers to the latest today and had a bunch of failing tests. This PR fixes those tests

mansona added a commit to empress/empress-blog-casper-template that referenced this pull request Jun 4, 2020
@mansona
Copy link
Contributor

mansona commented Jun 4, 2020

I've just come across this PR now too. I'm pretty sure #362 is a breaking change for anything ember-data related and not just relationships. The case that broke for me was just a simple:

{{#each (sort-by 'date:desc' this.model) as |post index|}}

so I'm not actually accessing anything via a relationship and I have the same regression.

@snewcomer if we aren't going ahead with this PR can we at least revert #362 and get a patch release out?

Copy link
Contributor

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

Nice fix @jrjohnson! I guess we will punt on the promise thing till later if ppl still want it. I'm assuming you do?

@snewcomer snewcomer merged commit 5ac366f into DockYard:master Jun 4, 2020
@jrjohnson jrjohnson deleted the fulfilled-promise-to-sort-by branch June 4, 2020 13:02
@jrjohnson
Copy link
Contributor Author

The promise thing was me over-simplifying the problem (incorrectly) and not actually something we need. This fix resolves all of our issues.

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.

4 participants