jquery
Syntax error unclear
We ran some front-end tests and saw the error:
Syntax error, unrecognized expression: #
We dug for a while looking for a stray # in our code before searching the internet deep enough to realize this was an invalid jQuery selector.
This error message would be more clear if it mentioned "sizzle", "jQuery", "selector", or another string that would make it clear to the user that this is not a JavaScript syntax error, but a sizzle selector expression syntax error.
@timmywil @dmethvin Do either of you foresee harm in changing the message (e.g., losing search results)? I would like to be more clear and concise, and seem to remember an existing ticket in jQuery that I can't find right now.
It was https://github.com/jquery/jquery/issues/1756, I don't think we ever settled on a better hook for someone trying to figure out where the problem is. I suppose we could always have jQuery.error call console.stack or something equally annoying to barf details out for debugging, but we've resisted such things in the past.
Yeah, that's it. Anyone who wants a stack trace is already free to pull it off the Error we throw, but our Syntax error, unrecognized expression: … messages leave much to be desired (as this ticket attests to). We're also inconsistent about when we throw (tokenize vs. compile), but that's a separate issue.
For this one, which duplicates jquery/jquery#1756 but is in the correct repository, it's mostly a question of better prefix text and (to a lesser extent) related concerns about what we show (how much of the selector, error-specific text, etc.). I'm of the opinion that error text in runtime libraries like Sizzle and jQuery bloats size at the expense of people doing things right, but at the same time understand its value for identifying and fixing mistakes (sometimes in third-party code). How about a concise uniform prefix and the offending simple selector (or invalid suffix, when the former cannot be identified)?
| Description | Selector | Error |
|---|---|---|
| Missing mandatory argument | #list > li:nth-child() .target |
Unsupported selector: :nth-child() |
| Prohibited argument | #container > :first-child( foo ) |
Unsupported selector: :first-child( foo ) |
| Unknown pseudo | :made-up, :input |
Unsupported selector: :made-up |
| Unimplemented pseudo | :visited |
Unsupported selector: :visited |
| Invalid argument | *:lang(en~us) |
Unsupported selector: :lang(en~us) |
| Parse failure | *: I |
Unsupported selector: : I (?) |
| Unknown combinator | Abbot & Costello |
Unsupported selector: & Costello |
In essence, we'd be hinting at the type of the problem and giving clues to track it down, but would not be specific in our complaints.
The error text might be customized in unminified builds only. As long as we throw in the same moments and just change the message in dev mode, we should be fine.
So you're suggesting different error messages for sizzle.js and sizzle.min.js? We could pull that off in this codebase, but it would impose complexity on downstream consumers like jQuery. It seems like you're unsatisfied with the proposal above, though... how would you like the (unminified) messages to differ?
I'm a little uncomfortable with introducing behavior changes between minified and unminified code, especially in a project intended for embedding.
I'm a little uncomfortable with introducing behavior changes between minified and unminified code, especially in a project intended for embedding.
Agreed. I think the main thing needed here is an error message that makes it clear it's a selector issue.
I'm a little uncomfortable with introducing behavior changes between minified and unminified code, especially in a project intended for embedding.
That may be a problem if someone minifies Sizzle by themselves which is a valid use case so maybe it's better to avoid it, indeed.
@gibson042 your proposal looks nice.
:+1: I also like "unsupported selector". The first word that came to mind after eventually discovering the cause of this error was "selector".
The first word that came to mind after eventually discovering the cause of this error was "selector".
My first thought would have started with "s" too but only has 4 letters.
