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

Fastboot compatibility #56

Merged
merged 1 commit into from
Jun 18, 2016
Merged

Conversation

anthonybalmeo
Copy link
Contributor

Adding Fastboot compatibility.

@runspired
Copy link
Collaborator

I'm willing to accept this, but I wonder if @tomdale has better recommended approaches. Looking up the service seems heavier than a typeof FASTBOOT !== 'undefined' check, which I've seen around a few places.

@arjansingh
Copy link

@runspired The FastBoot service and isFasBoot is the official public API for determining if the application is in FastBoot mode.

window.FastBoot is exposed as a config object inside the server side context, but that's private API and could change. I'll defer to Tom though.

@arjansingh
Copy link

You can check out the work we've been doing with Ember Simple Auth to see how we are handling it over there.

mainmatter/ember-simple-auth#964

@runspired
Copy link
Collaborator

@arjansingh I see. At a minimum I'd like to know why the service is being looked up in such a non-ember way.

@runspired
Copy link
Collaborator

Specifically:

Why something like this?

fastboot: computed(function() {
   let owner = getOwner(this).lookup('service:fastboot');
}),

Instead of this?

fastboot: inject.service('fastboot')

@arjansingh
Copy link

Because if an application is not requiring ember-cli-fasboot as a dependency, the injection will throw an error since the service won't exist. You can use Ember.inject.service but then you will have to make ember-cli-fasboot a required peer dependency for anyone using your addon.

@runspired
Copy link
Collaborator

@arjansingh thanks! This all makes much more sense now.

@anthonybalmeo thanks again for doing the legwork here :)

@runspired runspired merged commit 31ce7c7 into html-next:develop Jun 18, 2016
@arjansingh
Copy link

Believe me. I wish there was a cleaner way too. 😀 I'll let you know when I find one.

@anthonybalmeo
Copy link
Contributor Author

@runspired thanks!

@eriktrom
Copy link
Member

Many months ago this was not an issue, and injecting a service does not actually call init() so wondering if there is more context anyone could provide (at the least for the knowledge sharing sake).

This seems superfluous I guess IMO... Fastboot doesn't have an event dispatcher and it seems odd it would invoke such a code path. Anyone care to explain more if they have time. Likely a misunderstanding on my part so figured I'd ask the obvious.

@@ -29,6 +37,8 @@ export default Mixin.create({
},

__setupRecognizers: on('didInsertElement', function() {
if (this.get('__fastboot.isFastBoot')) { return; }
Copy link
Member

@eriktrom eriktrom Jun 22, 2016

Choose a reason for hiding this comment

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

Perhaps my question is related to this code path. Fastboot will not call this hook so... Curious I am if someone had an error that provoked this PR.

Thanks for hearing me out.

Choose a reason for hiding this comment

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

@eriktrom I was helping @anthonybalmeo on this. You're right, didInsertElement should not be called in FastBoot, but this code was somehow getting run, hence the guard. Maybe it is indirectly called by init like you suggest below. We're looking into that and will let you know.

@anthonybalmeo
Copy link
Contributor Author

Can you create a release for this? Thanks!

@eriktrom
Copy link
Member

@anthonybalmeo - window.Hammer is only invoked here when rendering in fastboot (within init) - https://github.com/runspired/ember-gestures/blob/31ce7c74bff5eadbfdfa7bcf4c4539ae85e310fa/addon/mixins/recognizers.js#L88

Hammer.js should not exist though anyway b/c of this guard I put in a while back within the build - https://github.com/runspired/ember-gestures/blob/31ce7c74bff5eadbfdfa7bcf4c4539ae85e310fa/index.js#L17-L24

To explain, didInsertElement is not called when rendering in fastboot. However init is called (first link above). The guard should be placed there if we're going to guard the world for fastboot - however i propose that a better way to handle this is for ember-gestures to only be included in the broccoli tree when outputting a browser build for an app consuming this addon (@tomdale confirm/deny?), and for it to not be included when outputting a fastboot build.

ember-network is a good example of this sort of branching - https://github.com/tomdale/ember-network/blob/2826247c5dd2bb2537497af5ad7fecbd3feba6c9/index.js#L49-L55

This way of handling fastboot shouldn't be a problem given ember-hammertime handles the ast transform of the rendered html from the server thus still providing 'fastclick' support before the app is replaced by the browser rendered version. (worth mentioning here that ember-hammertime should be in fastboot and browser build, if the ember-network inspired 'at build time' option becomes appealing to ya'll)

anyway, my 2 cents - figured i'd mention this cause I saw it on my phone yesterday but didn't have the time to explain what I was trying to invoke with the questions i asked

@anthonybalmeo I am curious what you've found in practice and what stack trace you got, that'll confirm or deny my suspicions (and/or craziness LOL)

k, cheers folks, let me know if I can help further

@arjansingh
Copy link

@eriktrom I think modifying the build tree approach would definitely be the cleanest. I don't know enough about ember-gesture's dependency structure to do that would probably let you remove all of the in code checks from this PR.

@arjansingh
Copy link

On Hammer existing or not, the issue isn't specifically about that, but rather that the code that calls Hammer is being executed server-side, hence the need for the guards.

@eriktrom
Copy link
Member

@anthonybalmeo - lets move this to #57 - gave the cleanup a shot

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

4 participants