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

Make addon more idiomatic at the expense of making it 2.10+ & fastboot 1.0rc+ #32

Merged
merged 3 commits into from
Jun 6, 2017

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented Jun 5, 2017

Added a note on the readme explaining that.

@kratiahuja
Copy link

Might as well move app/instance-initializer/fastboot/head.js to fastboot/instance-initializer/head.js since this is a major version bump ?

@cibernox
Copy link
Contributor Author

cibernox commented Jun 5, 2017

@kratiahuja I don't follow. That is precisely what I did.

@kratiahuja
Copy link

Super sorry :( those changes showed up very small in the diff so I got confused. This LGTM.

@simonihmig
Copy link
Contributor

@cibernox can you please explain the rationale for these changes? A bit of work went into #21 to not make any breaking changes, so it now runs with beta and rc version of FastBoot as well as those older Ember versions.

I understand the required code became quite a bit confusing when looking at it, but as this addon is probably used by most FastBoot apps, I think a higher degree of compatibility would make sense. At least until we feel safe to drop support for older version, say when FastBoot 1.0 stable has been released and most other addons have been migrated (still a long way, see ember-fastboot/ember-cli-fastboot#387). Then the changes here make absolutely sense to me!

@cibernox
Copy link
Contributor Author

cibernox commented Jun 5, 2017

I just asked @ronco if leaving 0.2.2 as a bridge version and release a simpler and leaner 0.3 version that is 1.0+ only was ok. He seemed to be ok, people using fastboot beta can or non-glimmer ember can continue to use the old version.

This PR also makes #30 irrelevant, so it's like 3 PRs in one.

@ronco
Copy link
Collaborator

ronco commented Jun 6, 2017

LGTM. @simonihmig are you ok with dropping pre-1.0 Fastboot support with a 0.3 version bump?

@simonihmig
Copy link
Contributor

@ronco Sure, if it's ok for you, so it is for me! :)

@rwjblue
Copy link
Member

rwjblue commented Jun 6, 2017

agreed, I like this direction

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.

5 participants