-
Notifications
You must be signed in to change notification settings - Fork 290
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
Do not throw for query() and queryAll() #39
Comments
Corresponding browser bugs: |
How do you envision jQuery's |
Though admittedly this isn't an exhaustive search, I'm having a hard time finding a popular JS library that relies on selectors throwing an error to implement their custom functionality. For instance, jQuery and MooTools both As with any proposed change, there is a compatibility risk to assess, but at first blush, I don't see any so large that this isn't worth discussing. |
If I look at the currently-most-recent jQuery 2 version (code at http://code.jquery.com/jquery-2.1.4.js) then on line 841 there is a Similarly the most recent jQuery 1 version at http://code.jquery.com/jquery-1.11.3.js has the same sort of setup, on lines 879 and 893 respectively. Looking at the file you linked to for jQuery, https://github.com/jquery/sizzle/blob/a7020477b0433ac08d66baf9eebb3f9d24868e21/src/sizzle.js#L308 has the corresponding call. If that succeeds, then As for mootools, the matching function you point to would probably still work, yes (it's using |
Yeah I don't think we can change this. We could change it for the new methods though... |
For the new methods not throwing on parse error might be pretty nice. As long as we also provide an explicit "is this selector supported?" API. And perhaps a general parsed-selector API... |
Yeah, I think I discussed a |
We could do performance measurements. The performance improvement from using a pre-parsed selector is quite noticeable in real-world cases. So browsers have internal parsed-selector caches, possibly with some sort of eviction heuristics, for the existing querySelectorAll methods. Exposing a |
Yeah, I think that's the conclusion we ended up with last time around, since UAs already have that hashtable for strings anyway. |
Avoiding the landmine that is trying a new selector in an old browser sounds useful to me. |
@tabatkins and @foolip, would appreciate your input here. |
I don't have much of an opinion on the API ergonomics of this. The biggest issue indeed would be code that depends on the exception being thrown, which normally isn't on the radar, but in this case looks like it is. We could measure how often the exception is thrown in Blink, but given how widely used the API itself is I'd be somewhat surprised if it's negligible. I'm not a big fan of making the prefixed and unprefixed APIs different, there's so much code assuming that they're equivalent, a difference here might be annoying to debug. Perhaps an API that returns null if the selector is supported and something non-null that can be passed to |
@foolip note that this would only affect the new APIs that are not implemented yet. |
Oh, I thought |
I'm somewhat in favor of a Selector object, but if we're avoiding that, then having query() and queryAll() just not throw would be fine. Re: a way to find if a selector is supported, I like foolip's idea - a |
|
Ah, yeah, empty string is better than null. |
Also, did error handling change? |
This issue is still open but I don't see |
They were commented out in 10b6cf1. |
@foolip thanks. Any chances this is coming back or is it not likely? (the only info I see is this comment). I'd like to update my answer on Stack Overflow on the difference between queryAll and querySelectorAll. If there is any information on how likely is queryAll to come back to the spec, I'd like to add it to that answer. Thanks. |
I suppose that implementer interest is needed. If the only difference between Also, one answer to the Stack Overflow question says that " |
Yes, we are waiting for subclassing support for built-ins in browsers and in IDL. |
Indeed, anything live is unlikely to get implemented ever I think :) |
That's the wrong conclusion. It's just taking a little longer as JavaScript subclassing of built-ins isn't baked yet. |
I finally wrote that |
@annevk Now that subclassing of built-ins is supported in most browsers, are there any plans to bring |
I think the main thing we're still waiting for is IDL support. There's whatwg/webidl#345 though that got hijacked a bit by a different issue. |
I don't know if IDL support is the real blocker. We could make it work, if there were implementer interest. I'm just not sure this better-ergonomics API is as high a priority for all browsers, sad to say :(. I'd welcome being corrected. |
I realized this issue is actually invalid as filed. The example given
never matches anything due to how selector parsing works. If a given selector in the comma-separated list is invalid, it's always a non-match. That's also true when you use such a selector in CSS. Combined with the seeming non-interest in |
If a given selector in CSS is invalid the entire rule is thrown out. That said, we could define different behavior from selectors API things, assuming the selector parsing spec gave us a way to parse a comma-separated selector list while only throwing out parts of the list. Or is the point that it doesn't? |
Yeah, it doesn't. |
I'd be fine with it doing something different. The Selectors error-recovery behavior is considered a legacy mistake; if we were writing it today we'd split on top-level commas and throw out only the entries we didn't recognize. querySelector() (or a future version) could do the same. |
Okay, worth keeping in mind I suppose if we ever add new methods. |
As the Web evolves, new selectors will be invented. (The CSS4 wiki contains 14 selector proposals.) As that happens, there will be selectors that are understood by some browsers but not yet others.
The current spec commands that browsers throw a SyntaxError when they don't understand a selector. Unless there is an actual syntax error (e.g. a mismatched paren), this should be handled more gracefully. Otherwise, because errors halt JS execution, any site that adopts a new selector is going to break in old browsers unless the developer knew and remembered to wrap the statement in a
try/catch
.This makes code both harder to read and harder to write without bugs. For instance, what should be this simple:
becomes this mess:
under the current spec.
The simplest solution would be to change the errors to warnings. This would be a welcome change, though it would make it harder to programmatically distinguish between unmatched and unsupported selectors. Perhaps a
navigator.supportsSelector
API should be added to make this differentiation easier.Whatever the solution, throwing an error on an unsupported selector is hostile to the future of the web. It should be fixed.
The text was updated successfully, but these errors were encountered: