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

Refactor initializer container lookups into an instanceIntializer #15

Merged
merged 3 commits into from
Jul 26, 2015
Merged

Refactor initializer container lookups into an instanceIntializer #15

merged 3 commits into from
Jul 26, 2015

Conversation

mike-north
Copy link
Contributor


//TextField/TextArea are currently uninjectable, so we're going to hack our
//way in
Ember.TextSupport.reopen({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't be on an InstanceInitializer, it should only be run once, not per instance.,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, good point. Overzealous copypasta FTL

// Set up a handler on the document for keyboard events that are not
// handled by Ember's event dispatcher.
Ember.$(document).on('keyup.outside_ember_event_delegation', null,
function(event) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also likely don't want to add multiple of these, do we tear them down?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe they're ever torn down. What's the best practice for an instance UNinitializer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't an instance initializer though, it should only occur once. That being said, it likely just be something the application view does, as that has the right life-cycle and scope.

Unfortunately, each approach appears pretty lame.

But for reference, the eventdispatcher (which this ultimately augments) is created at app boot, and removed at app teardown.

@lukemelia
Copy link
Contributor

I am correct that this breaks compatibility with Ember 1.10? That may be fine, I just want to verify so we can message accordingly.

@mike-north
Copy link
Contributor Author

I think we don't have to lose 1.10 compatibility if we're clever. My suggestion for moving forward is the following:

  • Add the instance initializer for any code that needs access to an instance of an app. This will be a dormant object in the container for apps running < 1.11.0, but it shouldn't be harmful
  • Add a condition, whereby for older ember versions, the entire initializer runs as it exists now. For >= 1.11.0, we'll only run part of the initializer, and the rest will be handled by the instanceInitializer.

I have built a thorough regex to extract the necessary version info from Ember.VERSION - http://www.regexr.com/3b274 since we seem to deviate from SemVer for certain versions made available through the release channel (i.e., 1.12.0.94b8cb6c does not seem to be valid according some implementations of SemVer parsing)

Thoughts @stefanpenner @lukemelia ? IMO the alternative is branching for pre/post 1.11.0

@lukemelia
Copy link
Contributor

@truenorth I'm +1 on your approach.

@mike-north
Copy link
Contributor Author

ping

lukemelia added a commit that referenced this pull request Jul 26, 2015
Refactor initializer container lookups into an instanceIntializer
@lukemelia lukemelia merged commit 4676bac into yapplabs:master Jul 26, 2015
@lukemelia
Copy link
Contributor

Thanks @truenorth!

@mike-north mike-north mentioned this pull request Aug 17, 2015
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