-
Notifications
You must be signed in to change notification settings - Fork 24
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
Ember 1.13 compatibility (fixes #8) #9
Conversation
The `registerComponentLookup` initializer is now an instance initializer, so we need to change the ember-islands boot initializer to an instance initializer too. Unfortunately there's no clean way to plumb the `bypass` config flag through to the instance, as instances do not have access to the parent application. To get around this we just set the config flag directly on the default/deprecated instance that exists in 1.13. Note that these changes mean that ember-islands will only work in Ember 1.13+.
Thanks so much for putting this together! I'm traveling right now but I should be able to review and merge Sunday. |
np! It's also likely that this is just a stopgap fix, until emberjs/ember.js#11440 is merged |
This looks good but I just ran with ember-try and got a failure in Canary. For some reason it can't load the files from the Chasing Canary is tough, I'm looking forward to having that container PR merged! |
@mitchlloyd - I'm seeing that same |
So the failure is apparently related to Ember Data not working in canary, which kills the rest of the build. Removing ED from package.json and bower.json fixes the failure. cc @hhff That being said, would we be able to get this merged as 0.6.0 (or a version of your choosing)? I'll come back and do a second pass once the container reform is done, which'll hopefully mean we can remove some of the hackier code. |
@praxxis This seems like a bug fix (no new features) so I was thinking about just a patch number bump. I'll remove Ember Data from I really wish all of the recent 1.x releases would just work and then we could break for 2.0. I'll look into adding more hacks to detect this particular case so that it can work with 1.13 and 1.12. |
Something using https://github.com/rwjblue/ember-cli-version-checker#real-world-example might be a solution? |
That's interesting. It seems like our own ad hoc feature detection here would probably not work since this is happening during Ember's boot process? @praxxis If you have the time would you mind putting something together? I'm moving this week and I'm really pressed for time. |
Sure thing
|
So a major problem is that the I'm going to play around with trying to change the initializer behaviour based on |
We now build the addon based on your Ember version, with builds prior to 1.13 maintaining the original initializer.
@mitchlloyd ok, this is what I ended up with. We now have pre- and post-fastboot initializers now. The code is duplicated but it seems clearer than trying to add in version logic, and the real work is done outside of the initializers anyway. Plus you only ship with the initializer that's applicable for your Ember version. |
word thanks @praxxis |
@praxxis This looks really great. Thanks for helping out with this! |
Yo @praxxis / @mitchlloyd - just put together an addon that might help here: https://github.com/hhff/ember-version-is |
The
registerComponentLookup
initializer is now an instance initializer, so we need to change the ember-islands boot initializer to an instance initializer too. Unfortunately there's no clean way to plumb thebypass
config flag through to the instance, as instances do not have access to the parent application. To get around this we just set the config flag directly on the default/deprecated instance that exists in 1.13.Note that these changes mean that ember-islands will only work in Ember 1.13+.
This was the simplest set of changes that got ember-islands working with 1.13, and setting
bypass
on the deprecated instance feels pretty hacky. In the future I could see us moving toward having some sort of "config" object in the application registry, that each instance could query for thebypass
flag.