make guards for fastboot

[eriktrom]

• add isBrowser() util
• add guards for fastboot within init calls inside components

I know you guys are doing a major upgrade right now, so no need to merge this, but the following allows the current master branch to work with fastboot. I figured while working on the next version, this would help keep that need in mind.

tl;dr - init() within components is called by fastboot. Other component hooks are not. This code wraps each call to init with a isBrowser() util. This includes mixins used by components.

Don't feel obligated to merge this. I will personally use these guards though, so if they break in any way, I'll file a follow up pr with a fix.

[DanChadwick]

Shouldn't we only add guards to things that we know are troublesome? Also, I think fastboot runs didRender. Maybe we should add comments to these hooks so that we are thinking about fastboot compatibility as we code?

/**
 * Implementation of init() hook.
 *
 * Also executed by fastboot on the server. Protect any browser specific code with `if (isBrowser()) ...`.
 */

Thoughts??

[miguelcobain]

I'd like to have this logic in a mixin if possible.

[DanChadwick]

I think some init code should run and some should be browser-only, no? If so, then how would a mix-in work?

[eriktrom]

@DanChadwick - so good point - this was a raw find/replace attempt just to make sure ember-paper would work with my fastboot setup - i didn't actually go through each init method till now

It seems if we move the init code I wrap to willRender in all the cases, everything should still work.

The only exception is the sniffer service, which we should probably wrap in ember-run-raf so we don't trigger a recalculate style between rendering frames. If we do so ember-run-raf has its own fastboot guard.

I can make those changes with the big caveat being 'why are we using init to do willRender's job' - if there is a real answer then I propose we step back and move the issue upstream b/c we'd be adding way to much tech debt as we'd always have to wrap all mixins init methods and component init methods with this, and even if we move to a mixin, we'd still have to remember to include the mixin that wraps those methods, which is far to brittle and risky.

Hopefully we can just move to using willRender - you guys mind taking a look through the changes here to make sure that'll work out for the codebase. I'll move them over if so and wrap sniffer in run raf. We'll be left with a clean way to guard fastboot and life will be good, so i hope that can work.

[eriktrom]

phrased some of that bad - the mixin paragraph should read - 'imagine tyring to remember to mixin the mixin that wraps init in both mixins we mixin to components and components own init methods when they do not import any mixins that wrap init'

haha, that is a funny sentence

[miguelcobain]

I was thinking something like (let me know if I'm delusional):

export default Mixin.create({
  init() {
    this._super(...arguments);
    if (isBrowser()) {
      this.initBrowser();
    }
  },

  initBrowser: Ember.K
});

export default Component.extend(FastbootMixin, {
  init() {
    this._super(...arguments);
    // run init code regardless of on browser or not
  },

  initBrowser() {
    // run init code on browser
  }
});

Like @eriktrom said (if I understood correctly), if there is already a Component hook that only runs on browsers, let's use it.

[eriktrom]

Yeah - willRender is called right after init and only on the browser - the only reason I've thought of (which i've never seen anyone do, and would required some major meta-programming + overwriting when init is called) would be if you called .create() on a component manually and tried to override the default behavior. That said, willRender is the way to go. I'll switch this PR up probably Monday. We can then also close #330 at that point.

[DanChadwick]

I'm confused. How does willRender help? It called upon every render and its called for fastboot.

The only component lifecycle hooks called in FastBoot are init, didReceiveAttrs, didUpdateAttrs, willRender and willUpdate.

[eriktrom]

Ah, thankyou - i just looked at my project - I use willInsertElement - my bad, wrong hook name, i knew it started with a W, sorry about that! - btw where did you look that up, docs or source code? The pattern then is old view hooks are not called in fastboot and the new component hooks are, something we should maybe put in the contributing guide for people.

Also given I was so excited the night I installed this lib and needed it to work with fastboot to play with it, I was so blatantly blut with my find/replace - I overrode way more init hooks - only 2 places actually touch window or document, which is our only concern.

addon/components/paper-autocomplete-list.js touches document and the sniffer service touches document (i'll double check that when i make the pr, that what i gleamed from looking at my commit here)

Also now i feel dumb, makes a good example though - its just so rare to touch the real dom I got it mixed up - but other people will too so we should keep an eye on that in PR's and add it to contrubuting.md

@DanChadwick - did i get that all right this time, LOL thanks for checking my work :)

[eriktrom]

Also willRender would have touched the DOM and caused perf issues as it can be called more than once, so double error - willInsertElement is only called once, just like init, phew glad we sorted that out

[DanChadwick]

The fastbook hook quote is from the readme. Scroll down to "No didInsertElement".
https://github.com/ember-fastboot/ember-cli-fastboot

Those calls may or may not be able to be moved from init to didInsertElement. didInsertElment can't set anything on the Object without arousing the deprecation gods.

[eriktrom]

Thanks for the link

[eriktrom]

Those calls may or may not be able to be moved from init to didInsertElement. didInsertElment can't set anything on the Object without arousing the deprecation gods.

This is true only when you call set on the Component - so I'll make sure to split calling set(if we do that) out from didInsertElement (although willInsertElement doesn't have that issue or does it - setting on an object after inserting the element causes the whole thing to repeat, recursively although they catch it and give you a warning but willInsertElement I don't recall, do you know?

[eriktrom]

So willInsertElement I use like this in my code, it works fine(and doesn't cause that issue)

  willInsertElement() {
    this.set('galleryElement', this.element);
    internals.onResize = () => this.onResize;
    window.addEventListener('resize', internals.onResize);
    ... snipped...
  },

[eriktrom]

I'll just give it a shot on Monday and we can go from there, thanks for the feedback @DanChadwick

[eriktrom]

(i'll also check the ember source code and tests for the lifecycle hooks to see what they fully do, its been a while since i reviewed them - good to understand whats happening under the hood for an addon used by lots of people)

[dustinfarris]

@eriktrom I am interested in getting fastboot support merged. is this PR still working for you? anything I can help with?

[dustinfarris]

any movement here?

[eriktrom]

@dustinfarris - sorry lack of movement on my part - feel free to take it and run, looks like your already headed that direction with your todo list - feel free to ping me for a review if you want though. And sorry for the delay in response.

tl;dr on what I'd love to see - use of willInsertElement for anything that touches document - in practice this shouldn't be too hard - but any mixins will have to be scoured for their use of document to ensure they abide by this idea as well.

I'll check back in on your todo list in a few - let me know if you want some help, I dropped the ball here 👎 but thanks for picking it up 👍

[dustinfarris]

I think this can be closed thanks to #430

/cc @miguelcobain