Bug Tracker

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#7389 closed enhancement (wontfix)

change .selector property into .selectors array

Reported by: cowboy Owned by:
Priority: low Milestone: 1.next
Component: selector Version: 1.4.3
Keywords: 1.7-discuss Cc:
Blocked by: Blocking:

Description (last modified by dmethvin)

I'm just throwing this out there, but based on questions I've received from people in the past and ticket #6754, I think it's potentially problematic for the .selector property to be a concatenated string of all previous selector strings / derived selectors, because people might think it's supposed to be a valid selector they can use to actually select elements.

In fact, other than for events, I don't see the value in the current .selector property's value, and even then, it's useless once any kind of complex traversing/filtering has taken place.

I propose that the .selector string property be changed to a .selectors array property, with the .selector property reflecting the last *actual* selector string used. For example:

var elems = $('div').children('p');

elems.selector // 'p'
elems.prevObject.selector // 'div'

elems.selectors // [ 'div', '.children(p)' ]
elems.prevObject.selectors // [ 'div' ]

This way, the array could just be joined if someone wants the old (current) behavior, but because it's an array it would allow a more robust programmatic after-the-fact selector parsing logic.

The .selectors array could even be an array of hashes, like so:

var elems = $('div').children('p');

elems.selector // 'p'
elems.prevObject.selector // 'div'

elems.selectors // [ { name: 'jQuery', selector: 'div' }, { name: 'children', selector: 'p' } ]
elems.prevObject.selectors // [ { name: 'jQuery', selector: 'div' } ]

Of course, at this point, since you already have the simple .selector property, you could forgo the array altogether and just add a .methodname property:

var elems = $('div').children('p');

elems.selector // 'p'
elems.prevObject.selector // 'div'

elems.methodname // 'children'
elems.prevObject.methodname // 'jQuery'

Either way, a non-string-property-approach might help avoid possible confusion from people who think that the .selector property is always a valid selector string they can use in a new selection, and might add some value to being able to better programmatically derive selectors from sequential complex/filtering operations.

Change History (18)

comment:1 Changed 9 years ago by snover

Owner: set to cowboy
Status: newpending

Other than avoiding any more confusion from people that ignore the documentation (which states “if DOM traversal methods have been called on the object, the string may not be a valid jQuery selector expression”), what sort of use cases are there for providing this?

comment:2 Changed 9 years ago by cowboy

Status: pendingnew

Benefits:

  • a slightly smaller and simpler jQuery
  • less end-user confusion (around both the .selector value and .live method behavior, or even the very simple but unexpected behavior in #6754)
  • with the array of hashes / .methodname property approach, the traversal/filtering methods and selectors used could be programmatically derived without complex string parsing

comment:3 Changed 9 years ago by SlexAxton

Component: unfiledselector
Priority: undecidedlow
Status: newopen

I think it's pretty fair to say that the current behavior doesn't make sense in a lot of common cases. In the case of just string concatenated dom traversal stuff, it's completely useless. I think, while this is largely unimportant since no one should be relying on the .selector property, that it couldn't hurt to consider it in a future build, just so we have something that can at least be consistent.

comment:4 Changed 9 years ago by dmethvin

Hmmm, I didn't even think we documented .selector publicly, but why the heck are people trying to look at it? We know it doesn't survive complex traversal or filtering methods. Saying "don't touch that" in the docs saves us the code of trying to fix it and still falling short.

comment:5 Changed 9 years ago by cowboy

The .selector property is documented here and has been for quite some time, IIRC.

comment:6 Changed 9 years ago by john

Milestone: 1.7
Owner: cowboy deleted
Status: openassigned

Let's look at this for 1.7.

comment:7 Changed 9 years ago by john

Milestone: 1.71.next

comment:8 Changed 8 years ago by john

Keywords: 1.7-discuss added

Nominating ticket for 1.7 discussion.

comment:9 Changed 8 years ago by Rick Waldron

Description: modified (diff)

-1, A huge burden that will require managing entries to the array... which means all existing methods would have to bear the addition of at least one function being added to their call stack. -9001

comment:10 Changed 8 years ago by jaubourg

-1, sounds like a cheap and horrible way to construct selection... and a means to actually do terribly wrong things in general

comment:11 Changed 8 years ago by timmywil

-1, might make selector more useful, but entirely unnecessary.

comment:12 Changed 8 years ago by dmethvin

Description: modified (diff)

-1, Not a fan of .selector at all.

comment:13 Changed 8 years ago by john

Description: modified (diff)

+1, I like this - it makes what we originally wanted to do with .selector actually possible (do all sorts of after-the-fact stack manipulation). This would be a very easy change given that we already handle all of this in pushStack.

comment:14 Changed 8 years ago by cowboy

Description: modified (diff)

I'd encourage everyone to look at #9469 and then from there, continue discussion of the .selector property. I think combining the ideas from that ticket with this one might be the best approach.

comment:15 Changed 8 years ago by scottgonzalez

Description: modified (diff)

+0, no opinion, this is all internal, right?

comment:16 Changed 8 years ago by ajpiano

Description: modified (diff)

-1, I like the idea of dropping this in #9469 and I definitely don't think we should spend our cycles working on getting this "right" if it's impossible.

comment:17 Changed 8 years ago by jzaefferer

Description: modified (diff)

+0

comment:18 Changed 8 years ago by dmethvin

Description: modified (diff)
Resolution: wontfix
Status: assignedclosed

Per the 1.7 vote this isn't going to be implemented.

Note: See TracTickets for help on using tickets.