-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[CLEANUP] Remove jQuery integration in EventDispatcher #19666
[CLEANUP] Remove jQuery integration in EventDispatcher #19666
Conversation
packages/@ember/-internals/glimmer/tests/integration/components/input-angle-test.js
Outdated
Show resolved
Hide resolved
assert.notOk(event.originalEvent, 'event is not a jQuery.Event'); | ||
} else { | ||
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we assert the presence of event
here and event instanceof Event
or something?
I believe these two comments apply across the rest of this file and some other files. In the test which isn't for deprecated behavior, please assert the event
argument is passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only does this totally make sense, but trying to add that assertion uncovered an actual bug, as the event passed was a jQuery event. 😱
The internal components seem do use some additional jQuery related magic, that I was not aware of:
ember.js/packages/@ember/-internals/glimmer/lib/components/internal.ts
Lines 556 to 576 in 3ce13ce
export function jQueryEventShim(target: DeprecatingInternalComponentConstructor): void { | |
if (JQUERY_INTEGRATION) { | |
let { prototype } = target; | |
let superListenerFor = prototype['listenerFor']; | |
Object.defineProperty(prototype, 'listenerFor', { | |
configurable: true, | |
enumerable: false, | |
value: function listenerFor(this: InternalComponent, name: string): EventListener { | |
let listener = superListenerFor.call(this, name); | |
if (jQuery && !jQueryDisabled) { | |
return (event: Event) => listener(new jQuery.Event(event)); | |
} else { | |
return listener; | |
} | |
}, | |
}); | |
} | |
} |
So this will need some more cleanup...
FYI - I moved this into draft so I can keep track of where review is needed vs where more work is in progress, please move back out of draft when its ready to review again. |
3175b8c
to
0d935ae
Compare
Pushed an update:
Note: there are still a few jQuery-related leftovers, like the deprecated feature flag, which are not directly related to the narrower scope of this PR, and which I plan to address in a follow-up PR when this is merged. |
Reopened #19552