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.

Sign up for GitHub

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

Jump to bottom

[CLEANUP] Remove jQuery integration in EventDispatcher #19666

Merged
merged 6 commits into from
Jul 23, 2021
Merged

[CLEANUP] Remove jQuery integration in EventDispatcher #19666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
28804ab
Remove jQuery integration in EventDispatcher
simonihmig May 24, 2021
d0dc039
Remove tests for jQuery events
simonihmig May 24, 2021
cfab47a
Fix tests after removed deprecations
simonihmig May 24, 2021
339670e
Remove jQueryEventShim for input components
simonihmig Jul 22, 2021
0d935ae
Remove jQueryEventShim from link-to
simonihmig Jul 22, 2021
2f787b8
Fix linting
simonihmig Jul 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ module.exports = {
Set: true,
Symbol: true,
WeakMap: true,
Event: true,
},

rules: {
Expand Down
6 changes: 0 additions & 6 deletions packages/@ember/-internals/glimmer/lib/components/abstract-input.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { TargetActionSupport } from '@ember/-internals/runtime';
import { TextSupport } from '@ember/-internals/views';
import { EMBER_MODERNIZED_BUILT_IN_COMPONENTS } from '@ember/canary-features';
import { assert } from '@ember/debug';
import { JQUERY_INTEGRATION } from '@ember/deprecated-features';
import { action } from '@ember/object';
import { isConstRef, isUpdatableRef, Reference, updateRef, valueForRef } from '@glimmer/reference';
import Component from '../component';
Expand All @@ -14,7 +13,6 @@ import InternalComponent, {
handleDeprecatedAttributeArguments,
handleDeprecatedEventArguments,
InternalComponentConstructor,
jQueryEventShim,
} from './internal';

const UNINITIALIZED: unknown = Object.freeze({});
Expand Down Expand Up @@ -243,8 +241,4 @@ export function handleDeprecatedFeatures(
});
}
}

if (JQUERY_INTEGRATION) {
jQueryEventShim(target);
}
}
24 changes: 0 additions & 24 deletions packages/@ember/-internals/glimmer/lib/components/internal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import { Owner, setOwner } from '@ember/-internals/owner';
import { guidFor } from '@ember/-internals/utils';
import { jQuery, jQueryDisabled } from '@ember/-internals/views';
import { EMBER_MODERNIZED_BUILT_IN_COMPONENTS } from '@ember/canary-features';
import { assert, deprecate } from '@ember/debug';
import { JQUERY_INTEGRATION } from '@ember/deprecated-features';
import {
CapturedArguments,
Destroyable,
Expand Down Expand Up @@ -552,25 +550,3 @@ if (EMBER_MODERNIZED_BUILT_IN_COMPONENTS) {
});
};
}

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;
}
},
});
}
}
6 changes: 0 additions & 6 deletions packages/@ember/-internals/glimmer/lib/components/link-to.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { TargetActionSupport } from '@ember/-internals/runtime';
import { isSimpleClick } from '@ember/-internals/views';
import { EMBER_MODERNIZED_BUILT_IN_COMPONENTS } from '@ember/canary-features';
import { assert, debugFreeze, deprecate, warn } from '@ember/debug';
import { JQUERY_INTEGRATION } from '@ember/deprecated-features';
import { EngineInstance, getEngineParent } from '@ember/engine';
import { flaggedInstrument } from '@ember/instrumentation';
import { action } from '@ember/object';
Expand All @@ -22,7 +21,6 @@ import InternalComponent, {
handleDeprecatedArguments,
handleDeprecatedAttributeArguments,
handleDeprecatedEventArguments,
jQueryEventShim,
opaquify,
} from './internal';

Expand Down Expand Up @@ -710,8 +708,4 @@ if (EMBER_MODERNIZED_BUILT_IN_COMPONENTS) {
}
}

if (JQUERY_INTEGRATION) {
jQueryEventShim(LinkTo);
}

export default opaquify(LinkTo, LinkToTemplate);
40 changes: 8 additions & 32 deletions packages/@ember/-internals/glimmer/tests/integration/components/input-angle-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { RenderingTestCase, moduleFor, runDestroy, runTask } from 'internal-test-helpers';
import { moduleFor, RenderingTestCase, runDestroy, runTask } from 'internal-test-helpers';
import { EMBER_MODERNIZED_BUILT_IN_COMPONENTS } from '@ember/canary-features';
import { action } from '@ember/object';
import { Checkbox, TextArea, TextField } from '@ember/-internals/glimmer';
import { set } from '@ember/-internals/metal';
import { TargetActionSupport } from '@ember/-internals/runtime';
import { getElementView, jQueryDisabled, jQuery, TextSupport } from '@ember/-internals/views';
import { getElementView, TextSupport } from '@ember/-internals/views';

import { Component } from '../../utils/helpers';

Expand Down Expand Up @@ -563,11 +563,7 @@ moduleFor(
actions: {
foo(value, event) {
assert.ok(true, 'action was triggered');
if (jQueryDisabled) {
assert.notOk(event.originalEvent, 'event is not a jQuery.Event');
} else {
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed');
}
Copy link
Member

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.

Copy link
Contributor Author

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...

assert.ok(event instanceof Event, 'Native event was passed');
},
},
});
Expand All @@ -589,11 +585,7 @@ moduleFor(
foo(value, event) {
triggered++;
assert.ok(true, 'action was triggered');
if (jQueryDisabled) {
assert.notOk(event.originalEvent, 'event is not a jQuery.Event');
} else {
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed');
}
assert.ok(event instanceof Event, 'Native event was passed');
},
},
});
Expand Down Expand Up @@ -663,11 +655,7 @@ moduleFor(
actions: {
foo(value, event) {
assert.ok(true, 'action was triggered');
if (jQueryDisabled) {
assert.notOk(event.originalEvent, 'event is not a jQuery.Event');
} else {
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed');
}
assert.ok(event instanceof Event, 'Native event was passed');
},
},
});
Expand All @@ -686,11 +674,7 @@ moduleFor(
actions: {
foo(value, event) {
assert.ok(true, 'action was triggered');
if (jQueryDisabled) {
assert.notOk(event.originalEvent, 'event is not a jQuery.Event');
} else {
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed');
}
assert.ok(event instanceof Event, 'Native event was passed');
},
},
});
Expand All @@ -710,11 +694,7 @@ moduleFor(
foo(value, event) {
triggered++;
assert.ok(true, 'action was triggered');
if (jQueryDisabled) {
assert.notOk(event.originalEvent, 'event is not a jQuery.Event');
} else {
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed');
}
assert.ok(event instanceof Event, 'Native event was passed');
},
},
});
Expand All @@ -740,11 +720,7 @@ moduleFor(
foo(value, event) {
triggered++;
assert.ok(true, 'action was triggered');
if (jQueryDisabled) {
assert.notOk(event.originalEvent, 'event is not a jQuery.Event');
} else {
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed');
}
assert.ok(event instanceof Event, 'Native event was passed');
},
},
});
Expand Down
37 changes: 6 additions & 31 deletions packages/@ember/-internals/glimmer/tests/integration/components/input-curly-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { RenderingTestCase, moduleFor, runDestroy, runTask } from 'internal-test
import { EMBER_MODERNIZED_BUILT_IN_COMPONENTS } from '@ember/canary-features';
import { action } from '@ember/object';
import { set } from '@ember/-internals/metal';
import { jQueryDisabled, jQuery } from '@ember/-internals/views';

import { Component } from '../../utils/helpers';

Expand Down Expand Up @@ -394,11 +393,7 @@ moduleFor(
actions: {
foo(value, event) {
assert.ok(true, 'action was triggered');
if (jQueryDisabled) {
assert.notOk(event.originalEvent, 'event is not a jQuery.Event');
} else {
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed');
}
assert.ok(event instanceof Event, 'Native event was passed');
},
},
});
Expand All @@ -420,11 +415,7 @@ moduleFor(
foo(value, event) {
triggered++;
assert.ok(true, 'action was triggered');
if (jQueryDisabled) {
assert.notOk(event.originalEvent, 'event is not a jQuery.Event');
} else {
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed');
}
assert.ok(event instanceof Event, 'Native event was passed');
},
},
});
Expand Down Expand Up @@ -494,11 +485,7 @@ moduleFor(
actions: {
foo(value, event) {
assert.ok(true, 'action was triggered');
if (jQueryDisabled) {
assert.notOk(event.originalEvent, 'event is not a jQuery.Event');
} else {
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed');
}
assert.ok(event instanceof Event, 'Native event was passed');
},
},
});
Expand All @@ -517,11 +504,7 @@ moduleFor(
actions: {
foo(value, event) {
assert.ok(true, 'action was triggered');
if (jQueryDisabled) {
assert.notOk(event.originalEvent, 'event is not a jQuery.Event');
} else {
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed');
}
assert.ok(event instanceof Event, 'Native event was passed');
},
},
});
Expand All @@ -541,11 +524,7 @@ moduleFor(
foo(value, event) {
triggered++;
assert.ok(true, 'action was triggered');
if (jQueryDisabled) {
assert.notOk(event.originalEvent, 'event is not a jQuery.Event');
} else {
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed');
}
assert.ok(event instanceof Event, 'Native event was passed');
},
},
});
Expand All @@ -566,11 +545,7 @@ moduleFor(
actions: {
foo(value, event) {
assert.ok(true, 'action was triggered');
if (jQueryDisabled) {
assert.notOk(event.originalEvent, 'event is not a jQuery.Event');
} else {
assert.ok(event instanceof jQuery.Event, 'jQuery event was passed');
}
assert.ok(event instanceof Event, 'Native event was passed');
},
},
});
Expand Down
Loading