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

Ember 1.13 compatibility (fixes #8) #9

Merged
merged 3 commits into from
Jun 25, 2015

Conversation

praxxis
Copy link

@praxxis praxxis commented Jun 19, 2015

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+.

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 the bypass flag.

Lindsey Smith added 2 commits June 19, 2015 14:00
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+.
@mitchlloyd
Copy link
Owner

Thanks so much for putting this together! I'm traveling right now but I should be able to review and merge Sunday.

@praxxis
Copy link
Author

praxxis commented Jun 19, 2015

np! It's also likely that this is just a stopgap fix, until emberjs/ember.js#11440 is merged

@mitchlloyd
Copy link
Owner

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 addon directory. Really weird because it works with 1.12.

Chasing Canary is tough, I'm looking forward to having that container PR merged!

@hhff
Copy link

hhff commented Jun 22, 2015

@mitchlloyd - I'm seeing that same /addon directory thing with my Ember Infinity addon against canary. going to do a sprint on it tonight but if you have any insight I'd love to hear it!

@praxxis
Copy link
Author

praxxis commented Jun 23, 2015

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.

@mitchlloyd
Copy link
Owner

@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 bower.json.

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.

@praxxis
Copy link
Author

praxxis commented Jun 23, 2015

Something using https://github.com/rwjblue/ember-cli-version-checker#real-world-example might be a solution?

@mitchlloyd
Copy link
Owner

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.

@praxxis
Copy link
Author

praxxis commented Jun 23, 2015

Sure thing

On Jun 23, 2015, at 7:36 PM, Mitch Lloyd notifications@github.com wrote:

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 https://github.com/praxxis If you have the time would you mind putting something together? I'm moving this week and I'm really pressed for time.


Reply to this email directly or view it on GitHub #9 (comment).

@praxxis
Copy link
Author

praxxis commented Jun 24, 2015

So a major problem is that the after property of the initializer and instance initializer is invalid/invalid when you switch versions (it's after registerComponentLookup in the 1.12 initializer, changing to the instance initializer in 1.13.) And booting fails hard if either after is invalid. And there's no way to feature detect for that, as the initializers need to be loaded to even see which ones are present.

I'm going to play around with trying to change the initializer behaviour based on Ember.VERSION but that feels really hacky :(

We now build the addon based on your Ember version, with builds prior to 1.13 maintaining the original initializer.
@praxxis
Copy link
Author

praxxis commented Jun 24, 2015

@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.

@hhff
Copy link

hhff commented Jun 24, 2015

word thanks @praxxis

@mitchlloyd mitchlloyd merged commit 66a1b26 into mitchlloyd:master Jun 25, 2015
@mitchlloyd
Copy link
Owner

@praxxis This looks really great. Thanks for helping out with this!

@hhff
Copy link

hhff commented Jun 26, 2015

Yo @praxxis / @mitchlloyd - just put together an addon that might help here: https://github.com/hhff/ember-version-is

bf4 added a commit to bf4/bf4.github.com that referenced this pull request Jun 29, 2015
bf4 added a commit to bf4/bf4.github.com that referenced this pull request Jul 1, 2015
bf4 added a commit to bf4/bf4.github.com that referenced this pull request Jul 1, 2015
@praxxis praxxis deleted the ember_1.13_compat branch August 19, 2015 15:32
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.

3 participants