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

Add component lookup fallback solution #145

Conversation

alexlafroscia
Copy link

I recently upgraded to the latest beta to fix the issue with app.import breaking, and found that the use of owner.hasRegistration broke Ember 1.13. I tried to come up with a failing test case, but was unable to do so, because I wasn't able to follow what situations that code was called under. If you could guide me toward how to make a test case, I'd love to add one, but this at least fixes the issues in my app.

@alexlafroscia alexlafroscia force-pushed the component-lookup-fallback-solution branch from 9ae375e to 49c5d87 Compare April 13, 2016 19:06
Using `owner.hasRegistration` does not work in Ember 1.13, so this falls
back to something supported if the original approach is unavailable.
@alexlafroscia alexlafroscia force-pushed the component-lookup-fallback-solution branch from 49c5d87 to 5fcb391 Compare April 13, 2016 19:07
@webark
Copy link
Owner

webark commented Apr 13, 2016

@alexlafroscia I thought that was what the get owner polyfill was doing.. but maybe not. I'm supprised there isn't a polyfill for this. And getting the testing back on track is something that needs to be handled. Im on vacation right now, so don't have a lot of time to look at it.

@alexlafroscia
Copy link
Author

alexlafroscia commented Apr 13, 2016

Oh, maybe? I thought that that polyfill just allowed you to call getOwner but if adding that to the app is the solution, we'll do that instead.

@alexlafroscia
Copy link
Author

It does look like that's what the ember-getowner-polyfill is supposed to do... Nonetheless, this change was required to make the latest beta of this addon work in Ember 1.13. Maybe there's something that needs to change in the polyfill, then?

@webark
Copy link
Owner

webark commented Apr 22, 2016

@alexlafroscia maybe. I remember that was what @rwjblue suggested. I'm still on vacation for another week, so won't have a chance to look at it until I'm back.

@LevelbossMike
Copy link

@alexlafroscia did you find another workaround for this or are you using a fork right now? I am also running into problems when using the addon in an 1.13.x-application.

@alexlafroscia
Copy link
Author

Aside from the one in the PR? No; we decided to install the add-on from my fork to get around the issue.

@webark
Copy link
Owner

webark commented Jun 15, 2016

@alexlafroscia is that ember try the new format? If it is, then everything looks fine and I can merge this and cut a release if you want. Sorry, forgot to pick this one back up after I got back!

@alexlafroscia
Copy link
Author

alexlafroscia commented Jun 15, 2016

@webark I think its the new format, but maybe I'm wrong? I don't think it matters if you export an object or a function. At this point, I honestly don't remember why I switched to a function, but I was probably following the guideline at the time ¯\_(ツ)_/¯ Honestly, at this point I've changed companies and no longer need this personally so if you want to ditch this PR, that's completely fine too.

@webark
Copy link
Owner

webark commented Jul 13, 2016

@alexlafroscia apparently you have to explicitly use the ember-getowner-polyfill. I'm getting testing going in PR #153 and fixed it in commit bf4aed1. Way to go automated testing! Pickup up it's first bug.

@webark
Copy link
Owner

webark commented Jul 13, 2016

going to be closing this in favor of #153 when it gets merged.

@webark webark closed this Jul 13, 2016
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.

None yet

3 participants