ember-fastboot / ember-cli-fastboot

Server-side rendering for Ember.js apps
http://ember-fastboot.com/
MIT License
851 stars 160 forks source link

Component className binding to controller property breaks Fastboot server #5

Closed vectart closed 8 years ago

vectart commented 9 years ago

I'm trying Fastboot for my e-commerce app: https://github.com/vectart/e-commerce-concept/tree/fastboot-bug

When the app is loaded in Fastboot-mode and I open http://localhost:3000/, the following error happens:

undefined:52562
      return sel ? jQuery['default'](sel, elem) : jQuery['default'](elem);
                                                                   ^
TypeError: Property 'default' of object #<Object> is not a function
    at Object.merge.default.$ (<anonymous>:52562:68)
    at CoreView.default.extend.$ (<anonymous>:53748:32)
    at null.<anonymous> (<anonymous>:47733:23)
    at Object.merge.default.invokeObserver (<anonymous>:52613:16)
    at stateCheckedFn (<anonymous>:54191:27)
    at Queue.invoke (<anonymous>:13359:18)
    at Object.Queue.flush (<anonymous>:13424:13)
    at Object.DeferredActionQueues.flush (<anonymous>:13229:19)
    at Object.Backburner.end (<anonymous>:12654:27)
    at Object.Backburner.run (<anonymous>:12709:20)

I've figured out that it caused by isActive:active className binding at https://github.com/vectart/e-commerce-concept/blob/fastboot-bug/app/components/product-details.js#L9.

This causes calling anonymous function at https://github.com/emberjs/ember.js/blob/5fd2d035b30aa9ebfe73de824b3b283ec8e589cc/packages/ember-views/lib/mixins/class_names_support.js#L123 and eventually fails in $ method https://github.com/emberjs/ember.js/blob/5fd2d035b30aa9ebfe73de824b3b283ec8e589cc/packages/ember-views/lib/views/states/has_element.js#L20

rwjblue commented 9 years ago

Great job tracking this down! I'll leave it up to @tomdale, but I think this will likely require fixing in ember itself (not this addon) so we may want to move the issue over there....

vectart commented 9 years ago

Thanks a lot in advance @rwjblue!

tomdale commented 9 years ago

Great catch @vectart. @rwjblue is right, this needs a fix in Ember. Ironically I just spent this week rewriting that code for Glimmer, so at the very least the issue will get resolved once that's merged in.

I'm curious, though, @vectart: the code you linked to is setting up an observer, and shouldn't fire unless the property changes. If the property is changing immediately during the first rendering pass, that indicates a potential performance problem. I bet if you resolved that, your app would make further progress booting in FastBoot.

vectart commented 9 years ago

@tomdale Thanks a lot for solving this issue, I'm looking forward to try these changes.

Actually, since we are loading index route, this isActive observer must not fire at all because it relates to product route and template: https://github.com/vectart/e-commerce-concept/search?utf8=%E2%9C%93&q=isActive

Is it an Ember-specific issue?

tomdale commented 9 years ago

@vectart Hmm, that is definitely the root cause of the issue. That will trigger an update to the DOM inside the first rendering pass, which will add somewhat significant slowdowns.

In general, doing things inside Ember.run.next should be considered a code smell. Can you say more about why this needs to be in run-loop scheduled code?

vectart commented 9 years ago

@tomdale It's hacky workaround to implement animations after route is loaded. Of course, it should be implemented in other way, for example using Liquid-Fire.

Nonetheless, I can't understand one thing.

That will trigger an update to the DOM inside the first rendering pass

Why does the first rendering pass on index route triggers render method of product route? Really, how this line of code might be called without loading product route? https://github.com/vectart/e-commerce-concept/blob/d87966737ab16317a692fef33263c2afb4fdb05f/app/routes/product.js#L13

vectart commented 9 years ago

@tomdale To confirm the bizarre behaviour, I'm logging render method call on product route: https://github.com/vectart/e-commerce-concept/blob/fastboot-bug/app/routes/product.js#L14

If the app is run by ember server and you open http://localhost:4200/, you'll see nothing in console. When you click any product thumbnail, you'll get Render product route in console.

But if it's started by ember fastboot and you open http://localhost:3000/, you'll get Render product route in terminal and app is crashed.

Why does Fastboot call render method of unnecessary route?

vectart commented 9 years ago

@tomdale Actually, index route works fine after I change the product dynamic route to static one:

  this.route('product', { path: '/artemis' });

Maybe { path: '/:id' } causes loading even on index route?

stefanpenner commented 9 years ago

@tomdale It's hacky workaround to implement animations after route is loaded. Of course, it should be implemented in other way, for example using Liquid-Fire.

Ember.run.schedule('afterRender' should be sufficient in nearly all cases, and no foreign async must enter

vectart commented 9 years ago

@stefanpenner Thank you, I'll use this nice approach. Maybe you could also clarify that bizarre loading of unnecessary route under Fastboot mode?

vectart commented 9 years ago

Using latest canary version, Ember.run.schedule('afterRender' still doesn't fix the issue. Thus, I call that code only if window.document is defined.

if (window.document) {
  Ember.run.schedule('afterRender', function () {
    controller.set('isActive', true);
  });
}
tomdale commented 9 years ago

@vectart The fact that it's different between FastBoot and in the browser is very surprising. I would need to investigate this in more detail, but this taking different paths is definitely a bug.

tomdale commented 9 years ago

@vectart Can you verify that this is still happening on Ember 2.0?

wagenet commented 9 years ago

I'm seeing this error with 2.1 beta:

TypeError: Cannot use 'in' operator to search for 'role' in undefined
    at Object.normalizeProperty (<anonymous>:12768:21)
    at createNonNamespacedAttrMorph (<anonymous>:57441:45)
    at Function.AttrMorph.create (<anonymous>:57436:14)
    at DOMHelper.prototype.createAttrMorph (<anonymous>:12220:32)
    at Object.buildRenderNodes (<anonymous>:56531:27)
    at Function.RenderResult.build (<anonymous>:56455:26)
    at render (<anonymous>:56418:37)
    at <anonymous>:57238:7
    at renderAndCleanup (<anonymous>:57273:18)
    at Block._firstRender (<anonymous>:57235:5)

It looks like it's related to attributes as well.

wagenet commented 9 years ago

AFAICT, this is because Ember is using a PhantomJS path for some of the code, which then causes failures since we're not in Phantom. I have no clue why it thinks we're in Phantom.

rwjblue commented 9 years ago

@wagenet - Beta's cannot be used with fastboot. You have to use canary until the feature has been approved (aka "go"ed). Can you double check your version? Also, in general, the stack trace that you shared seems to indicate that somewhere in normalizeProperty we are attempting to call 'role' in element or something where element is undefined.

wagenet commented 9 years ago

@rwjblue this is with canary. The element is undefined because we failed to look it up correctly. Lets discuss this further out of band as I'm not 100% sure this is even related to the originally reported issue.

tomdale commented 8 years ago

Closing due to inactivity. Please file a new issue if still a problem.