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

Update code to comply with Ember 2.8.0. #450

Merged
merged 2 commits into from
Jan 25, 2018

Conversation

josemariaruiz
Copy link
Contributor

Check discussion in emberjs/ember.js#14266

@jacobq
Copy link
Collaborator

jacobq commented Jan 25, 2018

Thanks for the PR. I just rebased your commit and plan to merge if the tests are still passing.

@jacobq
Copy link
Collaborator

jacobq commented Jan 25, 2018

Looking at this a little more closely, I actually have some concerns:

  • Does the alias alternative (computed + volatile) actually do the same thing? i.e. Will the property update when parentView.x changes?
  • We are using alias similarly in a number of other places (e.g. md-card-reveal.js has activator: alias('parentView.activator')) that are not changed in this PR. Why not make them all consistent?
  • If I understand correctly, the discussion you referenced appears to suggest that the root cause was actually a regression fixed by Fix release regressions. emberjs/ember.js#14281 and also that we might be doing something unsupported by trying to observe parentView (I'm hoping this is not the case but will have to check deprecations, warnings, functionality, etc. to know for sure -- I don't think there's currently test coverage for this).

@jacobq
Copy link
Collaborator

jacobq commented Jan 25, 2018

Thanks @grapho for helping me understand the problem better on the Ember community Slack chat. It sounds like .volatile() is really not what we're looking for here, but that we shouldn't be using parentView either.

you want to pass in attrs from parent views explicitly

I'll take a look to see if I can refactor things to eliminate use of parentView properties in components.

@jacobq jacobq force-pushed the master branch 3 times, most recently from 087ae95 to 78b4050 Compare January 25, 2018 23:00
@jacobq jacobq merged commit 8d124e7 into mike-north:master Jan 25, 2018
@mike-north-bot
Copy link
Collaborator

🎉 This PR is included in version 0.22.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants