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

Event handlers with invalid selectors should be caught at attach time #3071

Closed
fsateler opened this Issue Apr 21, 2016 · 13 comments

Comments

Projects
None yet
5 participants
@fsateler
Contributor

fsateler commented Apr 21, 2016

Otherwise, they become very difficult to debug.

See this jsfiddle for an example. Attaching an event handler with an invalid selector div:not causes an exception to be raised at event time. This makes it very difficult to debug, as the trace of the error will usually only include jquery code.

I suggest jQuery should throw an exception at attach time instead, which makes it fairly easy to detect the faulty code.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Apr 22, 2016

Member

I'm normally opposed to explicitly introducing exceptions, but eliminating this arbitrarily long delay between mistake and feedback seems really valuable. I recommend adding if ( selector ) { jQuery.find.matchesSelector( elem, selector ); } in jQuery.event.add. @fsateler, care to submit a PR?

Member

gibson042 commented Apr 22, 2016

I'm normally opposed to explicitly introducing exceptions, but eliminating this arbitrarily long delay between mistake and feedback seems really valuable. I recommend adding if ( selector ) { jQuery.find.matchesSelector( elem, selector ); } in jQuery.event.add. @fsateler, care to submit a PR?

@gibson042 gibson042 added the Event label Apr 22, 2016

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Apr 22, 2016

Member

I think we already received similar issues before - @dmethvin?

I would tend to agree with assumption here though.

Member

markelog commented Apr 22, 2016

I think we already received similar issues before - @dmethvin?

I would tend to agree with assumption here though.

@fsateler

This comment has been minimized.

Show comment
Hide comment
@fsateler

fsateler Apr 22, 2016

Contributor

@markelog Indeed, I searched but did not find it. I now see it here: #2720

The consensus there seems to be that the check should not be added. A claim was made that the console has the erroneous selector. This is not the case if the selector instead triggers a null reference exception or other error in jquery code (as the div:not selector does, this is probably a separate bug that needs fixing). Moreover, if the event was supposed to catch a special condition and prevent navigation, debugging becomes doubly hard.

Should I prepare and submit a PR?

Contributor

fsateler commented Apr 22, 2016

@markelog Indeed, I searched but did not find it. I now see it here: #2720

The consensus there seems to be that the check should not be added. A claim was made that the console has the erroneous selector. This is not the case if the selector instead triggers a null reference exception or other error in jquery code (as the div:not selector does, this is probably a separate bug that needs fixing). Moreover, if the event was supposed to catch a special condition and prevent navigation, debugging becomes doubly hard.

Should I prepare and submit a PR?

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 22, 2016

Member

I think it would be easier to see the advantages and drawbacks if there was a PR, so I wouldn't mind seeing a proposed solution with unit tests. One I can think of offhand is that custom selector extensions might not yet be in place, but that is probably rare.

Member

dmethvin commented Apr 22, 2016

I think it would be easier to see the advantages and drawbacks if there was a PR, so I wouldn't mind seeing a proposed solution with unit tests. One I can think of offhand is that custom selector extensions might not yet be in place, but that is probably rare.

@fsateler

This comment has been minimized.

Show comment
Hide comment
@fsateler

fsateler Apr 22, 2016

Contributor

ok, will prepare a PR

Contributor

fsateler commented Apr 22, 2016

ok, will prepare a PR

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog
Member

markelog commented Apr 22, 2016

@fsateler

This comment has been minimized.

Show comment
Hide comment
@fsateler

fsateler Apr 24, 2016

Contributor

Sorry for the newbie question, but... how do I run the testsuite? npm test does not appear to run the unit tests...

Contributor

fsateler commented Apr 24, 2016

Sorry for the newbie question, but... how do I run the testsuite? npm test does not appear to run the unit tests...

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 24, 2016

Member

@fsateler You need to have a working Apache+PHP setup (see https://github.com/jquery/jquery#running-the-unit-tests) and one domain pointed at the jQuery repository. Then open http://your-jquery-domain/test in a browser you want to check.

Member

mgol commented Apr 24, 2016

@fsateler You need to have a working Apache+PHP setup (see https://github.com/jquery/jquery#running-the-unit-tests) and one domain pointed at the jQuery repository. Then open http://your-jquery-domain/test in a browser you want to check.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Apr 24, 2016

Member

grunt runs a basic suite, and grunt test further includes Promises/A+ in Node.js (not affected here). But real proof of functionality takes place by loading our QUnit suite in-browser. Since this issue is tightly scoped, you can use ?module=event to speed things up, and even access directly from the filesystem if you don't want to set up a web server.

Member

gibson042 commented Apr 24, 2016

grunt runs a basic suite, and grunt test further includes Promises/A+ in Node.js (not affected here). But real proof of functionality takes place by loading our QUnit suite in-browser. Since this issue is tightly scoped, you can use ?module=event to speed things up, and even access directly from the filesystem if you don't want to set up a web server.

fsateler added a commit to fsateler/jquery that referenced this issue May 1, 2016

Event: evaluate selector at add time
This ensures events with invalid selectors throw at attach time instead
of event time.

Fixes: #3071

@fsateler fsateler referenced this issue May 2, 2016

Closed

Event: evaluate selector at add time #3097

3 of 4 tasks complete

@dmethvin dmethvin added this to the 3.0.0 milestone May 6, 2016

@dmethvin dmethvin assigned dmethvin and unassigned dmethvin May 6, 2016

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 6, 2016

Member

I'm labelling this as a behavior change since it has the potential to throw unexpected errors in the case where no event is delivered during a page's lifecycle. The PR looks good to me so I think we should land it in 3.0. Objections? If it lands I'll add a note to the upgrade guide.

Member

dmethvin commented May 6, 2016

I'm labelling this as a behavior change since it has the potential to throw unexpected errors in the case where no event is delivered during a page's lifecycle. The PR looks good to me so I think we should land it in 3.0. Objections? If it lands I'll add a note to the upgrade guide.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 6, 2016

Member

"Behavior Change" seems a bit much since this only affects invalid input, but I'm definitely in favor of including this fix in 3.0.

Member

gibson042 commented May 6, 2016

"Behavior Change" seems a bit much since this only affects invalid input, but I'm definitely in favor of including this fix in 3.0.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 6, 2016

Member

That's what we thought about [href=#] too. 😢 : The discussion there specifically pointed out they wanted to know cases where we start to throw and we didn't before, this is one of them. Better safe than sorry. I'll add a note in the upgrade guide too. Nobody should be complaining we changed behavior and didn't tell them!

Member

dmethvin commented May 6, 2016

That's what we thought about [href=#] too. 😢 : The discussion there specifically pointed out they wanted to know cases where we start to throw and we didn't before, this is one of them. Better safe than sorry. I'll add a note in the upgrade guide too. Nobody should be complaining we changed behavior and didn't tell them!

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 6, 2016

Member

Definitely. 👍

Member

gibson042 commented May 6, 2016

Definitely. 👍

@gibson042 gibson042 closed this in 7fd36ea May 7, 2016

@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.