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

Render for tagless component should not call view.$() #26

Closed
wants to merge 2 commits into from

Conversation

bantic
Copy link
Member

@bantic bantic commented Mar 16, 2015

In Ember 1.11, calling this.$() for a tagless component (tagName === ''), ember will throw an assertion complaining.

moduleForComponent always calls this.$() in its render callback, so in Ember 1.11 you can't effectively test the rendering of tagless components.

This adds an isolated commit with a failing test (and ember upgraded to 1.11.0-beta.5) and a second commit fixing that test.

I'm not sure if this is the best solution. It makes this.render simply return undefined for tagless components, which may be more surprising for a user than the assertion message that ember will raise.

Perhaps the render callback's call signature can change to include a shouldGetElement boolean instead?
So for a tagless component you would call this.render(false) in your test. And in that case render could raise its own assertion when called without false for a tagless component.

Alternatively, could use a heuristic to just find the first child of the ContainerView in the #ember-testing div, so that this.render always returns an element.

@rwjblue
Copy link
Member

rwjblue commented Mar 16, 2015

I think that I would prefer to simply remove the view.$() call all together. It would be a breaking API change, so we would have to bump ember-test-helpers to 0.4.0, but I think that it makes the most sense. If you need the element you can always call this.$() (which will need to be updated to call this.render(); this.subject.$())...

@rwjblue
Copy link
Member

rwjblue commented Mar 16, 2015

@dgeb - Do you have any objections to changing this.render() not return the element, and bumping the version accordingly?

@mixonic
Copy link
Sponsor Member

mixonic commented Mar 16, 2015

@rwjblue my thoughts are to decouple this from this.$().

var element = Ember.get(view, 'element');
if (element) {
  return jQuery(element);
}
return null;

@bantic
Copy link
Member Author

bantic commented Mar 16, 2015

@rwjblue I like that suggestion. I like @mixonic's suggestion for decoupling the this.$() but if a breaking change is on the table that makes the most sense to me, rather than making this.render() return a sometimes-null-sometimes-element.

If that sounds good to @dgeb I can rework this PR to move it to that.

@dgeb
Copy link
Member

dgeb commented Mar 16, 2015

I am not heavily invested in this issue, so if you all have reached consensus then I am 👍 as well.

@mixonic
Copy link
Sponsor Member

mixonic commented Mar 16, 2015

append would still return the element if available?

@mixonic
Copy link
Sponsor Member

mixonic commented Mar 16, 2015

Oh append has been removed. So it would, but would still raise a deprecation, and render would not. And this.$() would raise if you call it without an element I presume.

@bantic
Copy link
Member Author

bantic commented Mar 17, 2015

I made a PR that changes render to not call view.$() anymore, and had to make a few other small changes to make it work properly. #27

@bantic bantic closed this Mar 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.

None yet

4 participants