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

Allow addon to provide jQuery to final build, but not include Ember <-> jQuery integration. #148

Merged
merged 1 commit into from Oct 2, 2019

Conversation

lupestro
Copy link
Contributor

@lupestro lupestro commented Oct 1, 2019

Using a this.$() with explicit jquery-integration: false now invokes the code in emberjs proper. However, that emberjs code throws an assertion rather than displaying a deprecation when it detects the feature is disabled.

I'm of two minds whether this is appropriate. If it was an assertion because the flag indicated that jQuery would be absent so the call would fail anyway, it was only appropriate while the flag was closely coupled to the presence/absence of the package. If it interprets the explicit flag as saying, "As far as Ember is concerned, jQuery is no longer here," it might be quite appropriate.

I'm perfectly happy to leave this to wiser heads to sort out and implement what they recommend. One possibility is to provide an alternate implementation of component.dollar.js in this case that still applies the deprecation, but, knowing the jquery is present, doesn't resort to the assertion.

@lupestro
Copy link
Contributor Author

lupestro commented Oct 1, 2019

@rwjblue
Copy link
Member

rwjblue commented Oct 2, 2019

Using a this.$() with explicit jquery-integration: false now invokes the code in emberjs proper. However, that emberjs code throws an assertion rather than displaying a deprecation when it detects the feature is disabled.

This seems intentional. Specifying jquery-integration: false specifically means you want to opt out of Ember's jQuery APIs:

  • this.$() in Ember.Component's
  • Ember.$
  • Ember's jQuery based Event Dispatcher

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.

I think this looks good. Would like review from @simonihmig also (likely tomorrow), then can land and release. Thanks for working on it!

Copy link
Contributor

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @lupestro! 👍

@simonihmig simonihmig merged commit b2b5401 into emberjs:master Oct 2, 2019
@simonihmig
Copy link
Contributor

Merged, @rwjblue still needs to release!

@simonihmig
Copy link
Contributor

This seems like a breaking change (i.e. 0.7.0), as an app with integration turned off was still getting the (working) overridden this.$(), while now it will run into Ember's implementation that throws the assertion mentioned above, right?

@lupestro lupestro deleted the remove-deprecation-message branch October 2, 2019 12:37
@rwjblue
Copy link
Member

rwjblue commented Oct 2, 2019

This seems like a breaking change (i.e. 0.7.0), as an app with integration
turned off was still getting the (working) overridden this.$(), while now it will run into Ember's implementation that throws the assertion mentioned above, right?

Agreed, though (as I mentioned above) I think that is correct / intended behavior.

@rwjblue rwjblue changed the title Now suppressing component-dollar rather than raising a deprecation. Allow addon to provide jQuery to final build, but not include Ember <-> jQuery integration. Oct 2, 2019
@rwjblue rwjblue added the enhancement New feature or request label Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants