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

Avoid using jquery if disabled using flag #16687

Merged
merged 2 commits into from
May 27, 2018
Merged

Avoid using jquery if disabled using flag #16687

merged 2 commits into from
May 27, 2018

Conversation

cibernox
Copy link
Contributor

No description provided.

rwjblue
rwjblue requested changes May 25, 2018
View reviewed changes
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Also needs to add the flag to packages/ember-environment/lib/env.ts (like this one)...

@cibernox cibernox force-pushed the dont-use-jquery-if-disabled-using-flag branch from 60b6a16 to 03a33dd Compare May 25, 2018 14:08
@cibernox
Copy link
Contributor Author

@rwjblue Done!

This was referenced May 25, 2018
Closed
Merged
simonihmig
simonihmig reviewed May 26, 2018
View reviewed changes
packages/ember-views/lib/system/jquery.js

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

rwjblue
rwjblue reviewed May 26, 2018
View reviewed changes
packages/ember-views/lib/system/event_dispatcher.js Outdated
import ActionManager from './action_manager';
import fallbackViewRegistry from '../compat/fallback-view-registry';

const HAS_JQUERY = jQuery !== undefined;
const HAS_JQUERY = jQueryDisabled === false;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should check for false here (though I suppose it doesn't matter too much). In general we can trust that our jQueryDisabled export will always be boolean and will encapsulate the proper "default to true, unless false" logic.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, any reason to not just use jQueryDisabled in this file instead of making the second constant (HAS_JQUERY)?

rwjblue
rwjblue requested changes May 26, 2018
View reviewed changes
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Can you also update:

ember.js/packages/@ember/application/tests/application_test.js

Line 372 in d97dcf7

if (jQuery) {

ember.js/packages/ember/index.js

Line 564 in d97dcf7

Ember.$ = views.jQuery;

(only setup Ember.$ if the jQuery is enabled)

@cibernox cibernox force-pushed the dont-use-jquery-if-disabled-using-flag branch from 49441f7 to de4d6fd Compare May 26, 2018 19:49
@rwjblue
Copy link
Member

rwjblue commented May 26, 2018

Darn, sorry. My last change suggestion made the reexport tests fail

ember.js/packages/ember/tests/reexports_test.js

Line 171 in 4aed456

['$', 'ember-views', 'jQuery'],

Should become:

JQueryDisabled && ['$', 'ember-views', 'jQuery']

Then add a .filter(Boolean) to the end of that array. This will make it only check for Ember.$ export when it should be there.

Sorry for the run around...

@cibernox cibernox force-pushed the dont-use-jquery-if-disabled-using-flag branch from de4d6fd to bae56f9 Compare May 26, 2018 21:08
rwjblue
rwjblue approved these changes May 26, 2018
View reviewed changes
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you!!

@cibernox cibernox force-pushed the dont-use-jquery-if-disabled-using-flag branch from bae56f9 to 4aa5c84 Compare May 26, 2018 21:15
@cibernox
Copy link
Contributor Author

I had to rebase because conflicts with the deprecation of originalEvent, but it should be green now.

rwjblue
rwjblue reviewed May 26, 2018
View reviewed changes
Copy link
Member

@rwjblue rwjblue left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks like a logging test is failing? kk

@cibernox
Copy link
Contributor Author

@simonihmig After rebasing over your deprecating a test is failing, but I think I've resolved conflicts correctly. Can you take a look to see if it's evident to your what is going on?

simonihmig
simonihmig reviewed May 26, 2018
View reviewed changes
packages/@ember/application/tests/application_test.js Outdated
if (jQuery) {
assert.equal(messages[2], 'jQuery : ' + jQuery().jquery);
if (jQueryDisabled) {
assert.equal(messages[2], 'my-lib : ' + '2.0.0a');
assert.equal(messages[3], 'my-lib : ' + '2.0.0a');
Copy link
Contributor

Choose a reason for hiding this comment

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

@cibernox this is related to the failing test, and it seems this line has to go into the else-block (which is when jQuery is enabled)! (as you have switched the order of the jQuery/no-jQuery blocks)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, but it wasn't related with the failing test apparently.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it did, as the previous failed test was related to the env output, and that seems to be passing now. But a deprecation test is now failing: https://travis-ci.org/emberjs/ember.js/jobs/384219437#L1562 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the only failure I was seeing before. That was the one I don't see why it is failing because of my changes

@cibernox cibernox force-pushed the dont-use-jquery-if-disabled-using-flag branch from 4aa5c84 to 0e00d48 Compare May 26, 2018 22:43
@simonihmig
Copy link
Contributor

@cibernox Not sure what's going on. I would guess it's something about the changes around jQueryDisabled, that the tests expect the deprecation but it's not triggered. I am looking at ENV directly here, so I could change that in some test. Don't if it's related. I would have to run the tests in the browser through the debugger, to really understand what's happening there. But not able to do so today, but I can have a look on Monday, if you don't beat me to it...

@rwjblue
Copy link
Member

rwjblue commented May 27, 2018

I just pushed a commit that should fix the remaining issue which was basically that we didn't properly identify the difference between jQuery being explicitly enabled (which should not receive a deprecation) and when it was implicitly enabled (which should receive a deprecation).

@rwjblue rwjblue merged commit fd44f81 into emberjs:master May 27, 2018
@simonihmig
Copy link
Contributor

@rwjblue thanks a lot! 👍

@cibernox cibernox deleted the dont-use-jquery-if-disabled-using-flag branch May 27, 2018 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

@simonihmig simonihmig simonihmig left review comments

@locks locks locks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cibernox @rwjblue @simonihmig @locks