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

jquery-integration optional feature prints a warning even when enabled #16849

Closed
boris-petrov opened this issue Jul 30, 2018 · 11 comments · Fixed by emberjs/ember-optional-features#24

Comments

@boris-petrov
Copy link
Contributor

boris-petrov commented Jul 30, 2018

I've followed all the steps that are needed for enabling jQuery integration (I have the latest versions of @ember/jquery and @ember/optional-features installed and in package.json; have "jquery-integration": true in optional-features.json) but I still see a warning in the console when accessing originalEvent. I see the following check:

return EmberENV._JQUERY_INTEGRATION === true;

Which returns false in my case as EmberENV is:

{"FEATURES":{},"EXTEND_PROTOTYPES":false,"_APPLICATION_TEMPLATE_WRAPPER":false,"_TEMPLATE_ONLY_GLIMMER_COMPONENTS":true}

I think _JQUERY_INTEGRATION defaults to true and perhaps it is being removed from the hash because it has the same value? If this is so, the code above should be changed to:

return EmberENV._JQUERY_INTEGRATION !== false;

Am I missing something?

P.S. Ember 3.3.1 and latest Ember CLI.

@pixelhandler
Copy link
Contributor

@boris-petrov so you have a optional-features.json file with 'jquery-integration': true and you still see a warning? Can you make an example app that reproduces the issue and link here?

@boris-petrov
Copy link
Contributor Author

@pixelhandler - now that I think about it, this warning comes from a third-party plugin - namely ember-drag-drop. There is actually an issue opened there. But in any case - I do have optional-features.json with 'jquery-integration': true, yes. It doesn't warn me when I use originalEvent in my code but it doesn't suppress the issue in plugins. Is that expected and is there something I can do (apart from waiting for the addon to be updated)?

@boris-petrov
Copy link
Contributor Author

Actually, I lied. This also warns in our own code. I have something like:

{{input value=this.filterText keyDown=(action "keyDown") keyUp=(action "keyUp") focusIn=(action "focused") focusOut=(action "unfocused")}}
  actions:
    unfocused: (event) ->
      target = event.relatedTarget ? event.originalEvent?.explicitOriginalTarget

And this gives the warning. Please tell me if you cannot reproduce it with this code and I will try to create a full sample.

@simonihmig
Copy link
Contributor

I can confirm things behave strangely here. Will investigate...

@simonihmig
Copy link
Contributor

I think _JQUERY_INTEGRATION defaults to true and perhaps it is being removed from the hash because it has the same value?

Exactly: https://github.com/emberjs/ember-optional-features/blob/master/index.js#L75-L78

This basically makes it impossible to distinguish between jQuery being enabled by default and enabled explicitly. But we rely on this distinction here:
https://github.com/emberjs/ember.js/blob/master/packages/ember-views/lib/system/jquery_event_deprecation.js#L24-L31

I would say we should fix this in ember-optional-features. I can create a PR if we agree. @rwjblue your thoughts?

@boris-petrov
Copy link
Contributor Author

@simonihmig - why do you prefer to fix this in ember-optional-features and not what I've suggested in the original post? I think this would also work.

@simonihmig
Copy link
Contributor

@boris-petrov EmberENV._JQUERY_INTEGRATION !== false would also trigger the deprecation when the user explicitly opted into having jQuery (by setting the jquery-integration to true), which we want to prevent, in accordance to the RFC. If he explicitly wants to use jQuery, he should be able to do so, including using originalEvent and that without any deprecations.

@boris-petrov
Copy link
Contributor Author

@simonihmig - the documentation for deprecate specifies that only falsy values would trigger the deprecation. _JQUERY_INTEGRATION being either true or undefined would result in EmberENV._JQUERY_INTEGRATION !== false to be true (as both are not equal to false) which will not trigger the assertion. Only a value of false for EmberENV._JQUERY_INTEGRATION would result in this expression to be false and hence show the deprecation. Or am I missing something?

@simonihmig
Copy link
Contributor

_JQUERY_INTEGRATION being either true or undefined would result in EmberENV._JQUERY_INTEGRATION !== false to be true (as both are not equal to false) which will not trigger the assertion.

But we want to show the deprecation if the flag is undefined, and only then!

We don't show it when it's either true (user explicitly wants jQuery) or false (their is no jQuery available in the first place, so any use of $, .originalEvent or whatever else comes from jQuery would break the code anyway).

But because of the mentioned behavior in ember-optional-features we have no way to distinguish at runtime if the user did not set it (it's undefined), or he/she explicitly set it to true.

@bendemboski
Copy link
Contributor

FWIW, here's a repo with a failing test for the issue.

simonihmig added a commit to simonihmig/ember-optional-features that referenced this issue Sep 14, 2018
This is to enable the Ember runtime to distinguish between `true` by default and explicitly `true`. Which is required to correctly implement the `jQuery.originalEvent` deprecation, where this should only be triggered when jQuery is enabled by default, and not when it is explicitly enabled. Previously `EmberENV['_JQUERY_INTEGRATION']` would be `true` for both cases, with no way to distinguish. Now when the flag is not explicitly set `EmberENV['_JQUERY_INTEGRATION']` will be `undefined`.

The default value will now only be used for the `isFeatureEnabled()` check, but not for populating `EmberENV` anymore.

Fixes emberjs/ember.js#16849
@simonihmig
Copy link
Contributor

@bendemboski Thanks for the reproduction, that was helpful!

I created a PR for ember-optional-features that fixes the problem: emberjs/ember-optional-features#24

simonihmig added a commit to simonihmig/ember-optional-features that referenced this issue Sep 14, 2018
This is to enable the Ember runtime to distinguish between `true` by default and explicitly `true`. Which is required to correctly implement the `jQuery.originalEvent` deprecation, where this should only be triggered when jQuery is enabled by default, and not when it is explicitly enabled. Previously `EmberENV['_JQUERY_INTEGRATION']` would be `true` for both cases, with no way to distinguish. Now when the flag is not explicitly set `EmberENV['_JQUERY_INTEGRATION']` will be `undefined`.

The default value will now only be used for the `isFeatureEnabled()` check, but not for populating `EmberENV` anymore.

Fixes emberjs/ember.js#16849
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants