Refactor setupRenderingContext so that element is stable after setup.
#282
Conversation
Previously (with `moduleForComponent`) eager rendering was avoided
because it was not clear that rendering was _actually_ desired (and
therefore the extra work would be wasted in cases that were not testing
rendering). Unlike `moduleForComponent` we can be certain that
`setupRenderingContext` is only used in actual rendering contexts (its
in the name :P ), so we do not need to be concerned with the
performance.
This change is a compatible change (unless someone was _relying_ on
the fact that `this.element` was undefined, which seems super weird)
that ensures `this.element` is set immediately upon
`setupRenderingContext` being called (instead of lazily when the first
`render` is done). At that point `this.element` is essentially an empty
element (roughly akin to having only `{{outlet}}` in an `application`
template without an `index` template).
This unlocks the ability to test pending promises during rendering (via
things like `waitUntil`).
|
|
||
| if (global.jQuery) { | ||
| context.$ = function $(selector) { | ||
| // emulates Ember internal behavor of `this.$` in a component | ||
| // https://github.com/emberjs/ember.js/blob/v2.5.1/packages/ember-views/lib/views/states/has_element.js#L18 | ||
| return selector ? global.jQuery(selector, element) : global.jQuery(element); | ||
| return selector ? global.jQuery(selector, context.element) : global.jQuery(context.element); |
There was a problem hiding this comment.
Unrelated, but I just learned that context.element is a jQuery element if you have jQuery. Is that intended?
I'd have expeted user to do the wrapping themselves if they want to, like
import $ from 'jquery';
// ...
assert.equal($(this.element).hasClass('foo'));
There was a problem hiding this comment.
@cibernox this.$ = $(this.element), this.element is always a native DOM element
There was a problem hiding this comment.
@cibernox - That is very surprising indeed! As you can see (just above in the diff) this is how we set context.element now (was the same before but done via Object.defineProperty):
context.element = document.querySelector('#ember-testing > .ember-view');How could that ever be a jQuery element? Are you certain that this wasn't in a moduleForComponent test?
There was a problem hiding this comment.
Nevermind. I read this ternary completely wrong. I thought the line was setting context.element to global.jQuery(context.element).
Totally agreed. A number of conditionals removed... |
| // Does not use `await` intentionally | ||
| let renderPromise = render(hbs`{{promise-wrapper promise=promise }}`); | ||
|
|
||
| await waitFor('.loading'); |
There was a problem hiding this comment.
This is exactly what I was looking for two days ago 🎉
|
@rwjblue yes, it does. |
Previously (with
moduleForComponent) eager rendering was avoided because it was not clear that rendering was actually desired (and therefore the extra work would be wasted in cases that were not testing rendering). UnlikemoduleForComponentwe can be certain thatsetupRenderingContextis only used in actual rendering contexts (its in the name :P ), so we do not need to be concerned with the performance.This change is a compatible change (unless someone was relying on the fact that
this.elementwas undefined, which seems super weird) that ensuresthis.elementis set immediately uponsetupRenderingContextbeing called (instead of lazily when the firstrenderis done). At that pointthis.elementis essentially an empty element (roughly akin to having only{{outlet}}in anapplicationtemplate without anindextemplate).This unlocks the ability to test pending promises during rendering (via things like
waitUntil).Fixes #281