Skip to content
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

Tweak addEvent so it silently ignores non-eventing contexts #486

Closed
wants to merge 2 commits into from

Conversation

jespersm
Copy link

@jespersm jespersm commented Nov 4, 2013

I was trying to use a recent QUnit within Rhino (for https://github.com/finnjohnsen/jstest), but couldn't, since the simple context set up in Rhino does not have events, and a very simple 'root' context object.

This tiny change made 1.12.0 usable for my purpose, but it could be that I'm just doing it wrong.

@JamesMGreene
Copy link
Member

@Krinkle: I thought we fixed this in Portland...?

Looks like the Travis build failed.

@jespersm
Copy link
Author

jespersm commented Nov 6, 2013

I didn't try this in the HEAD version, so if you've fixed this since 0.12.0, my apologies.

@JamesMGreene
Copy link
Member

@jespersm: Nope, you're in the right. It's still broken in the current mainline.

@Krinkle
Copy link
Member

Krinkle commented Nov 7, 2013

@JamesMGreene Do we want to land another inline detection hack or shall we wait for your nodejs-compat branch to land? Alternatively we could also wait for #405 at which point all browser/html related stuff will be in a code path that doesn't get run under nodejs in the first place.

If the CLA is signed and we squash the broken commit before merging, this is good to go as far as I'm concerned 👍

@JamesMGreene
Copy link
Member

I don't mind if this gets landed since I have to totally rebase my PR anyway given the codebase split.

@jzaefferer
Copy link
Member

I've made this fix in da6d7cd

@jespersm could you sign our CLA for future contributions? http://contribute.jquery.org/CLA/

FYI @JamesMGreene @Krinkle running node test/node-test.js currently fails, since the require points at the wrong file. Even when fixing the path, the test just does nothing. We should address that.

@jzaefferer jzaefferer closed this Dec 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants