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

Avoid using jquery if disabled using flag #16687

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions packages/@ember/application/tests/application_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { libraries } from 'ember-metal';
import { getDebugFunction, setDebugFunction } from '@ember/debug';
import Application from '..';
import { Router, NoneLocation, Route as EmberRoute } from 'ember-routing';
import { jQuery } from 'ember-views';
import { jQueryDisabled, jQuery } from 'ember-views';
import { _loaded } from '@ember/application';
import Controller from '@ember/controller';
import { Object as EmberObject } from 'ember-runtime';
Expand Down Expand Up @@ -369,11 +369,11 @@ moduleFor(
this.runTask(() => this.createApplication());

assert.equal(messages[1], 'Ember : ' + VERSION);
if (jQuery) {
if (jQueryDisabled) {
assert.equal(messages[2], 'my-lib : ' + '2.0.0a');
} else {
assert.equal(messages[2], 'jQuery : ' + jQuery().jquery);
assert.equal(messages[3], 'my-lib : ' + '2.0.0a');
} else {
assert.equal(messages[2], 'my-lib : ' + '2.0.0a');
}

libraries.deRegister('my-lib');
Expand Down
1 change: 1 addition & 0 deletions packages/ember-environment/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from './lib/context';
export * from './lib/env';
export { default as global } from './lib/global';
14 changes: 14 additions & 0 deletions packages/ember-environment/lib/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,20 @@ export const ENV = {
*/
_TEMPLATE_ONLY_GLIMMER_COMPONENTS: false,

/**
Whether the app is using jQuery. See RFC #294.

This is not intended to be set directly, as the implementation may change in
the future. Use `@ember/optional-features` instead.

@property _JQUERY_INTEGRATION
@for EmberENV
@type Boolean
@default true
@private
*/
_JQUERY_INTEGRATION: true,

// the following for addon support
_ENABLE_EMBER_K_SUPPORT: false,
_ENABLE_SAFE_STRING_SUPPORT: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {
} from '@ember/instrumentation';
import { EMBER_IMPROVED_INSTRUMENTATION } from '@ember/canary-features';
import { jQueryDisabled, jQuery } from 'ember-views';
import { ENV } from 'ember-environment';
import { HAS_NATIVE_PROXY } from 'ember-utils';
import { DEBUG } from '@glimmer/env';

Expand Down Expand Up @@ -334,11 +333,11 @@ if (jQueryDisabled) {
'EventDispatcher#jquery-events',
class extends RenderingTest {
beforeEach() {
this.jqueryIntegration = ENV._JQUERY_INTEGRATION;
this.jqueryIntegration = window.ENV._JQUERY_INTEGRATION;
}

afterEach() {
ENV._JQUERY_INTEGRATION = this.jqueryIntegration;
window.ENV._JQUERY_INTEGRATION = this.jqueryIntegration;
}

['@test jQuery events are passed when jQuery is present'](assert) {
Expand Down Expand Up @@ -412,7 +411,7 @@ if (jQueryDisabled) {
assert
) {
let receivedEvent;
ENV._JQUERY_INTEGRATION = true;
window.ENV._JQUERY_INTEGRATION = true;

this.registerComponent('x-foo', {
ComponentClass: Component.extend({
Expand Down
4 changes: 2 additions & 2 deletions packages/ember-testing/lib/setup_for_testing.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* global self */

import { setTesting } from '@ember/debug';
import { jQuery } from 'ember-views';
import { jQuery, jQueryDisabled } from 'ember-views';
import { getAdapter, setAdapter } from './test/adapter';
import {
incrementPendingRequests,
Expand Down Expand Up @@ -32,7 +32,7 @@ export default function setupForTesting() {
setAdapter(typeof self.QUnit === 'undefined' ? new Adapter() : new QUnitAdapter());
}

if (jQuery) {
if (!jQueryDisabled) {
jQuery(document).off('ajaxSend', incrementPendingRequests);
jQuery(document).off('ajaxComplete', decrementPendingRequests);

Expand Down
139 changes: 69 additions & 70 deletions packages/ember-views/lib/system/event_dispatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@ import { assign } from '@ember/polyfills';
import { assert } from '@ember/debug';
import { get, set } from 'ember-metal';
import { Object as EmberObject } from 'ember-runtime';
import jQuery from './jquery';
import jQuery, { jQueryDisabled } from './jquery';
import ActionManager from './action_manager';
import fallbackViewRegistry from '../compat/fallback-view-registry';
import addJQueryEventDeprecation from './jquery_event_deprecation';

const HAS_JQUERY = jQuery !== undefined;
const ROOT_ELEMENT_CLASS = 'ember-application';
const ROOT_ELEMENT_SELECTOR = `.${ROOT_ELEMENT_CLASS}`;

Expand Down Expand Up @@ -144,32 +143,7 @@ export default EmberObject.extend({

let rootElementSelector = get(this, 'rootElement');
let rootElement;
if (HAS_JQUERY) {
rootElement = jQuery(rootElementSelector);
assert(
`You cannot use the same root element (${rootElement.selector ||
rootElement[0].tagName}) multiple times in an Ember.Application`,
!rootElement.is(ROOT_ELEMENT_SELECTOR)
);
assert(
'You cannot make a new Ember.Application using a root element that is a descendent of an existing Ember.Application',
!rootElement.closest(ROOT_ELEMENT_SELECTOR).length
);
assert(
'You cannot make a new Ember.Application using a root element that is an ancestor of an existing Ember.Application',
!rootElement.find(ROOT_ELEMENT_SELECTOR).length
);

rootElement.addClass(ROOT_ELEMENT_CLASS);

if (!rootElement.is(ROOT_ELEMENT_SELECTOR)) {
throw new TypeError(
`Unable to add '${ROOT_ELEMENT_CLASS}' class to root element (${rootElement.selector ||
rootElement[0]
.tagName}). Make sure you set rootElement to the body or an element in the body.`
);
}
} else {
if (jQueryDisabled) {
if (typeof rootElementSelector !== 'string') {
rootElement = rootElementSelector;
} else {
Expand Down Expand Up @@ -208,6 +182,31 @@ export default EmberObject.extend({
rootElement.tagName}). Make sure you set rootElement to the body or an element in the body.`,
rootElement.classList.contains(ROOT_ELEMENT_CLASS)
);
} else {
rootElement = jQuery(rootElementSelector);
assert(
`You cannot use the same root element (${rootElement.selector ||
rootElement[0].tagName}) multiple times in an Ember.Application`,
!rootElement.is(ROOT_ELEMENT_SELECTOR)
);
assert(
'You cannot make a new Ember.Application using a root element that is a descendent of an existing Ember.Application',
!rootElement.closest(ROOT_ELEMENT_SELECTOR).length
);
assert(
'You cannot make a new Ember.Application using a root element that is an ancestor of an existing Ember.Application',
!rootElement.find(ROOT_ELEMENT_SELECTOR).length
);

rootElement.addClass(ROOT_ELEMENT_CLASS);

if (!rootElement.is(ROOT_ELEMENT_SELECTOR)) {
throw new TypeError(
`Unable to add '${ROOT_ELEMENT_CLASS}' class to root element (${rootElement.selector ||
rootElement[0]
.tagName}). Make sure you set rootElement to the body or an element in the body.`
);
}
}

let viewRegistry = this._getViewRegistry();
Expand Down Expand Up @@ -239,45 +238,7 @@ export default EmberObject.extend({
return;
}

if (HAS_JQUERY) {
rootElement.on(`${event}.ember`, '.ember-view', function(evt) {
let view = viewRegistry[this.id];
let result = true;

if (view) {
result = view.handleEvent(eventName, addJQueryEventDeprecation(evt));
}

return result;
});

rootElement.on(`${event}.ember`, '[data-ember-action]', evt => {
let attributes = evt.currentTarget.attributes;
let handledActions = [];

evt = addJQueryEventDeprecation(evt);

for (let i = 0; i < attributes.length; i++) {
let attr = attributes.item(i);
let attrName = attr.name;

if (attrName.lastIndexOf('data-ember-action-', 0) !== -1) {
let action = ActionManager.registeredActions[attr.value];

// We have to check for action here since in some cases, jQuery will trigger
// an event on `removeChild` (i.e. focusout) after we've already torn down the
// action handlers for the view.
if (action && action.eventName === eventName && handledActions.indexOf(action) === -1) {
action.handler(evt);
// Action handlers can mutate state which in turn creates new attributes on the element.
// This effect could cause the `data-ember-action` attribute to shift down and be invoked twice.
// To avoid this, we keep track of which actions have been handled.
handledActions.push(action);
}
}
}
});
} else {
if (jQueryDisabled) {
let viewHandler = (target, event) => {
let view = viewRegistry[target.id];
let result = true;
Expand Down Expand Up @@ -350,6 +311,44 @@ export default EmberObject.extend({
});

rootElement.addEventListener(event, handleEvent);
} else {
rootElement.on(`${event}.ember`, '.ember-view', function(evt) {
let view = viewRegistry[this.id];
let result = true;

if (view) {
result = view.handleEvent(eventName, addJQueryEventDeprecation(evt));
}

return result;
});

rootElement.on(`${event}.ember`, '[data-ember-action]', evt => {
let attributes = evt.currentTarget.attributes;
let handledActions = [];

evt = addJQueryEventDeprecation(evt);

for (let i = 0; i < attributes.length; i++) {
let attr = attributes.item(i);
let attrName = attr.name;

if (attrName.lastIndexOf('data-ember-action-', 0) !== -1) {
let action = ActionManager.registeredActions[attr.value];

// We have to check for action here since in some cases, jQuery will trigger
// an event on `removeChild` (i.e. focusout) after we've already torn down the
// action handlers for the view.
if (action && action.eventName === eventName && handledActions.indexOf(action) === -1) {
action.handler(evt);
// Action handlers can mutate state which in turn creates new attributes on the element.
// This effect could cause the `data-ember-action` attribute to shift down and be invoked twice.
// To avoid this, we keep track of which actions have been handled.
handledActions.push(action);
}
}
}
});
}
},

Expand All @@ -373,12 +372,12 @@ export default EmberObject.extend({
return;
}

if (HAS_JQUERY) {
jQuery(rootElementSelector).off('.ember', '**');
} else {
if (jQueryDisabled) {
for (let event in this._eventHandlers) {
rootElement.removeEventListener(event, this._eventHandlers[event]);
}
} else {
jQuery(rootElementSelector).off('.ember', '**');
}

rootElement.classList.remove(ROOT_ELEMENT_CLASS);
Expand Down
7 changes: 4 additions & 3 deletions packages/ember-views/lib/system/jquery.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { context } from 'ember-environment';
import { hasDOM } from 'ember-browser-environment';
import { ENV } from 'ember-environment';

let jQuery;
export let jQueryDisabled = false;
export let jQueryDisabled = ENV._JQUERY_INTEGRATION === false;

if (hasDOM) {
jQuery = context.imports.jQuery;

if (jQuery) {
if (!jQueryDisabled && jQuery) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how this is all connected, but I see a few modules importing jQuery from this module, without checking for jQueryDisabled. Before this change, there was no case possible that jQuery was available while jQueryDisabled is true. This is now possible, with the flag being false but the jQuery global still available. So should we override jQuery to undefined, if jQueryDisabled is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonihmig I think you might be right. Not if the user has explicitly disabled jQuery, Ember.$ will be undefined, even if jQuery happens to be included in the page in some other way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, I agree. I think that we need to change all of our guards to be checking jQueryDisabled instead of the default export of this module.

tldr; it should absolutely be possible to tell Ember to treat jQuery as disabled even if window.$ is present...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue I already did in this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh! sorry :( I guess I thought we had more places that did the conditional...

if (jQuery.event.addProp) {
jQuery.event.addProp('dataTransfer');
} else {
Expand All @@ -25,4 +26,4 @@ if (hasDOM) {
}
}

export default jQuery;
export default (jQueryDisabled ? undefined : jQuery);
13 changes: 11 additions & 2 deletions packages/ember-views/lib/system/jquery_event_deprecation.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* global Proxy */
import { deprecate } from '@ember/debug';
import { ENV } from 'ember-environment';
import { global } from 'ember-environment';
import { HAS_NATIVE_PROXY } from 'ember-utils';
import { DEBUG } from '@glimmer/env';

Expand All @@ -20,7 +20,16 @@ export default function addJQueryEventDeprecation(jqEvent) {
case 'originalEvent':
deprecate(
'Accessing jQuery.Event specific properties is deprecated. Either use the ember-jquery-legacy addon to normalize events to native events, or explicitly opt into jQuery integration using @ember/optional-features.',
ENV._JQUERY_INTEGRATION === true,
(EmberENV => {
// this deprecation is intentionally checking `global.EmberENV` /
// `global.ENV` so that we can ensure we _only_ deprecate in the
// case where jQuery integration is enabled implicitly (e.g.
// "defaulted" to enabled) as opposed to when the user explicitly
// opts in to using jQuery
if (typeof EmberENV !== 'object' || EmberENV === null) return false;

return EmberENV._JQUERY_INTEGRATION === true;
})(global.EmberENV || global.ENV),
{
id: 'ember-views.event-dispatcher.jquery-event',
until: '4.0.0',
Expand Down
4 changes: 3 additions & 1 deletion packages/ember/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,9 @@ Object.defineProperty(Ember, 'TEMPLATES', {
Ember.VERSION = VERSION;

// ****ember-views****
Ember.$ = views.jQuery;
if (!views.jQueryDisabled) {
Ember.$ = views.jQuery;
}
Ember.ViewUtils = {
isSimpleClick: views.isSimpleClick,
getViewElement: views.getViewElement,
Expand Down
5 changes: 3 additions & 2 deletions packages/ember/tests/reexports_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import Ember from '../index';
import { FEATURES } from '@ember/canary-features';
import { confirmExport } from 'internal-test-helpers';
import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
import { jQueryDisabled } from '../../ember-views';

moduleFor(
'ember reexports',
Expand Down Expand Up @@ -168,7 +169,7 @@ let allExports = [
['Logger', 'ember-console', 'default'],

// ember-views
['$', 'ember-views', 'jQuery'],
!jQueryDisabled && ['$', 'ember-views', 'jQuery'],
['ViewUtils.isSimpleClick', 'ember-views', 'isSimpleClick'],
['ViewUtils.getViewElement', 'ember-views', 'getViewElement'],
['ViewUtils.getViewBounds', 'ember-views', 'getViewBounds'],
Expand Down Expand Up @@ -302,4 +303,4 @@ let allExports = [
// ember-extension-support
['DataAdapter', 'ember-extension-support'],
['ContainerDebugAdapter', 'ember-extension-support'],
];
].filter(Boolean);