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
Syntax error unclear #326
Comments
It was jquery/jquery#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 |
Yeah, that's it. Anyone who wants a stack trace is already free to pull it off the Error we throw, but our 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)?
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. |
Agreed. I think the main thing needed here is an error message that makes it clear it's a selector issue. |
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. |
Yeah i like "Unsupported selector", says what it means. |
👍 I also like "unsupported 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. |
obviously the issue of selector module |
jQuery version of this issue: jquery/jquery#1756 |
We ran some front-end tests and saw the error:
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.
The text was updated successfully, but these errors were encountered: