[FEATURE ember-views-tagless-jquery] add support for this.$() in tagless components

[miguelcobain]

Tests are green with and without features flags enabled.
This seems to be the most concise way of returning a range of objects in jquery.

Please let me know if something's missing.

[rwjblue]

can you explain this? Doesn't 'jQuery(sel, range)' do the work we need? I guess what I'm asking is, what is the result of the '.add(range.filter(sel))' bit?

[miguelcobain]

It's a bit tricky. Example:
Suppose the sollowing HTML

<ul>
  <li></li>
  <li class="second-li"></li>
  <li></li>
</ul>

Suppose var lis is the result of $('li').
If you then to $('.second-li', lis), the second li won't be returned. Basically, jquery "finds" if the inner elements match the selector, not if elements themselves match.

.add(range.filter(sel)) will make sure we also consider the topmost elements that match sel.

[rwjblue]

Looks pretty good to me. I'll try to play with it a bit more tonight.

One thing we have to make sure is that all new features are properly documented. Can you add API docs here and review the guides repo to check if any changes are needed there?

[miguelcobain]

@rwjblue I've added a note on the API docs.
Will review the guides tomorrow.
Also changed the if's to become more friendly with the babel plugin.

[btecu]

Views are back? 👻

[miguelcobain]

@btecu Components extend Views, currently: https://github.com/emberjs/ember.js/blob/master/packages%2Fember-views%2Flib%2Fcomponents%2Fcomponent.js#L118

[rwjblue]

In general we need at least two core team members to 👍 to merge a new feature (behind a flag), and then later we vote to enable or not.

This looks good to me.

@mixonic - Mind merging if this looks good to you also?

[rwjblue]

@miguelcobain - Sadly, I just caused a merge conflict for you in FEATURES.md, would you mind rebasing?

[miguelcobain]

Done.

[chancancode]

I believe @wycats has some doubts about this, specifically around the selector stability (e.g. what if the first/last nodes are dynamic ({{...}}) or helper/component invocations?

[rwjblue]

@chancancode - Great points

@miguelcobain - Can you add a test that has the first and last nodes as invoked components using:

• the {{component helper
• bound properties (possibly as {{{rawHTML}}})

[miguelcobain]

There's an "issue".

If the template is {{component "foo-bar"}}<span></span>, this unexpectedly leads to 3 elements:
the firstNode is an empty text node, and only then comes the foo-bar component DOM and the <span>.
The same happens if {{component is last.

Is this expected? Don't really know how this text node is coming up.

[miguelcobain]

I've updated the PR.
The current implementation creates an ELEMENTS symbol to avoid adding a discoverable property on Component.
It also "paves the way" to this.elements(). this.$() is implemented using the elements method.
In a nutshell:

• it ignores comment and empty text nodes for first and last nodes.
• it ignores comment nodes in between

The reason for not ignoring empty text nodes in between is basically a "conservative" measure, i.e ignore the least possible.
Unfortunately I'm afraid there is no way to avoid ignoring comment nodes in between.

/cc @rwjblue @chancancode

[miguelcobain]

Please let me know if anything is missing here.

[rwjblue]

@chancancode - Tests were added to address the concerns you listed above. Mind reviewing again?

I'm game to let this simmer on canary as a feature while we play with the functionality. We can always decide to remove if we have specific issues about go forward compatibility...

[mixonic]

I am 👍. Looks good!

[rwjblue]

This needs to be tweaked a bit:

// at the top
import buildOwner from 'container/tests/test-helpers/build-owner';
import { OWNER } from 'container/owner';

var owner = buildOwner();
owner.register(/* stuff here */);

// later after registrations
View.extend({
  [OWNER]: owner
})

[wycats]

I'm a little bit concerned about adding such a significant feature that will constrain backwards compatibility at this point.

I'm also concerned about the fact that we're changing the meaning of this.$ in a subtle way, so that people who come to a component that happens to be tagless (or people who write generic conveniences for working with components) suddenly have to worry about the change. In addition to the fact that there can now be more than one "root" element, the fact that the jQuery object can now contain text nodes is a somewhat significant change from what people are used to.

I don't think we need to rush this right now. There are a number of view-layer balls in the air at the moment and we should revisit this once they settle.

[homu]

☔ The latest upstream changes (presumably #12673) made this pull request unmergeable. Please resolve the merge conflicts.

[miguelcobain]

@homu, with the latest comment by @wycats I was wondering if this is still a desired feature or not.

I will wait for clearer instructions. I can rebase this PR anytime.

[krisselden]

@wycats I kind of like this, I think though it should be clear in the documentation that .$() array like is not maintained, it will be a snapshot.

[miguelcobain]

LOL, I just noticed how much of a jerk I looked like replying to @homu.

[runspired]

I'm willing to create a legacy polyfill for this if we land it. @krisselden @wycats

As far as subtle difference concerns go, luckily you usually know you are working in a tagless component, since you had to explicitly set it to be so.

The alternative could be something like this.elementRange and this.$range(), which could then be valid for tagless and non-tagless components. I think if we did that though, everyone might just always use the range version for consistency, and we'd be no better off than having dropped notes in the documentation.

[pixelhandler]

@miguelcobain is this PR inactive? Was the another resolution for this issue?

[mrtnpro]

Any updates on this PR? @miguelcobain

[miguelcobain]

@crtvhd the core team didn't seem to approve this PR (or at least it looked so to me).

With the recent efforts to make ember jquery independent, I don't know if this is the way anymore.
Maybe we could use DOM ranges? https://developer.mozilla.org/en-US/docs/Web/API/Range

[mrtnpro]

@miguelcobain Apparently! Which is a bummer as I just ran into this problem: Toggling isVisible on a tag-less Ember.Component which is basically the same as accessing this.$ . Guess I have to get creative now :)

Ranges sound like they might be in fact a good fit here. Nonetheless I think we should close this PR since it wasn't accepted. In case you or anyone else wants to propose another approach using Ranges you/they can open a new PR.

[tomdale]

@miguelcobain Sorry that we never gave you a clear direction on this. We just discussed how to move forward at the core team meeting and, while we decided against accepting this PR, we agree this is an important use case and want to expose it in a first-class way.

There are a few reasons we didn't want to add support for this.$() to tagless components:

1. There is a lot of interest in and on-going work on removing Ember's dependency on jQuery. Coupling this to this.$() means that anyone opting out of jQuery would be missing important functionality.
2. The implementation in this PR effectively creates a "snapshot" of all of the elements in the range, but those elements are unstable and may change over time. We're afraid that this may be surprising to users, because the jQuery object returned by this.$() for normal components is "live", but if you hold on to the jQuery object returned by this.$() for tagless components, it may give you out-of-date elements.

Glimmer already has an internal, first-class notion of stable "bounds" objects that point to a first and last element. In Ember, these are exposed via the (admittedly still private) Ember.ViewUtils.getViewBounds() and getViewRange(), which today are used by Liquid Fire and the Ember Inspector to interoperate with tagless components.

Based on today's discussion, we think the best path forward is to work on a design that exposes the notion of a component's bounds as first-class, public API. If that existed, it would hopefully make implementing the jQuery-based functionality of this PR pretty straightforward for an addon.

Thank you very much for the PR and for prompting this discussion!