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

Make jQuery optional #294

Merged
merged 4 commits into from Feb 4, 2018
Merged

Make jQuery optional #294

merged 4 commits into from Feb 4, 2018

Conversation

simonihmig
Copy link
Contributor

@rwjblue
Copy link
Member

rwjblue commented Jan 12, 2018

Big 👍 here, thank you for taking the time to work on it!


To not create any breaking changes, Ember CLI will have to check the app's dependencies for the presence of this addon.
If it is not present, it will continue importing jQuery *unless* the jQuery integration flag is disabled.
In the other case, it will stop importing jQuery at all, and delegate this responsibility to the addon.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

If it is present,

That's what I meant with In the other case, but I can rephrase this.


To nudge users to install `@ember/jquery` when they need jQuery, some warning/deprecation messages should be issued when
the addon is *not* installed and the integration flag is either not specified or is set to true. Only in the case the
app is actively opting out of jQuery integration the addon is not needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

The @ember/jquery addon should be placed in the default blueprint for a period of time so as to ease migration.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, absolutely agreed.

should throw an assertion stating that jQuery is not available.
* `this.$()` in components
should throw an assertion stating that jQuery is not available and that `this.element` and native DOM APIs should be
used instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these throw deprecation warnings before jquery is removed?

Copy link
Member

Choose a reason for hiding this comment

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

No, I don't think so. Importantly, this RFC is not suggesting that usage of jQuery specific features are deprecated. It's providing for a way to opt-out of that feature set which is coherent.

There certainly may be an additional deprecation RFC down the road, but for now we are trying to ensure that either option (using jQuery integration features or not including jQuery) are viable coherent options for Ember apps...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. I had a second follow up RFC planned, that is more aggressive (and thus maybe controversial) about pushing away from jQuery, including deprecations as mentioned. But this would be complimentary to this one, so basically the second step (if adopted).

### Guides

The existing "Managing Dependencies" chapters in the Ember Guides as well as on ember-cli.com provide a good place to
explain users how to set the jQuery integration flag by means of the mentioned addon that handles this flag.
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify this is not the @ember/jquery addon

Copy link
Member

Choose a reason for hiding this comment

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

Yes, great point. Currently we are thinking that it will be @ember/optional-features (WIP repo here)...


For this reason this RFC encourages addon authors to not use jQuery anymore and to refactor existing usages whenever
possible! This certainly does not apply categorically to all addons, e.g. those that wrap jQuery plugins as
components and as such cannot drop this dependency.
Copy link
Contributor

Choose a reason for hiding this comment

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

Addons that require jquery should state that they do so.

At some point the end goal should be that such addons could simply include @ember/jquery in their dependencies and not have additional instructions.

Copy link
Member

Choose a reason for hiding this comment

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

Addons that require jquery should state that they do so.

Totally agreed! The current line of thinking is that addons that require jQuery (or any of the other optional features that we are adding opt-out capabilities for) can as @ember/optional-features if any given feature is enabled or disabled, this allows an addon that is (for example) wrapping a jQuery plugin ensure that jQuery integration is enabled (and @ember/jquery is present properly).

At some point the end goal should be that such addons could simply include @ember/jquery in their dependencies and not have additional instructions.

Possibly, but for now I think that we want to leave control of opting in or out of features in the hands of the application specifically.


## Unresolved questions

### jquery.Event vs. native events
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, this question needs to be somewhat resolved before this RFC can proceed, as it pretty much derails the whole show.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I agree that we need to reach a consensus on the best path forward here.

accessed. However this can be misleading, as it may be required to use these jQuery specifics if a jQuery event is
passed, so there is technically no other way to prevent the deprecation warning to be triggered.
* is there a way to start passing *only* native events, which are augmented by jQuery specific properties like
`originalEvent` (e.g. `event.originalEvenet = event`) to not break things, thus following SemVer while still
Copy link
Member

Choose a reason for hiding this comment

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

typo: s/originalEvenet/originalEvent/

@rwjblue
Copy link
Member

rwjblue commented Jan 13, 2018

RE: The jQuery.Event vs native event situation, I think we have additional options that are not listed. The simplest solution that comes to my mind is:

Create a small addon that detects if jQuery integration is enabled or disabled (at build time), and provide a consistent interface to always convert to a native event. Something like:

// jQuery enabled version
export default function(jQueryEvent) {
  return jQueryEvent.originalEvent;
}
// jQuery disabled version
export default function(event) { 
  return event;
}

This utility function could be used where needed like:

import Component from '@ember/component';
import normalizeEvent from '@ember/some-awesome-name';

export default Component.extend({
  click(event) {
    let nativeEvent = normalizeEvent(event);
    // do stuff...
  }
})

I think there are a number of variations of this setup that would work nicely...

@simonihmig
Copy link
Contributor Author

simonihmig commented Jan 13, 2018

Create a small addon that detects if jQuery integration is enabled or disabled (at build time), and provide a consistent interface to always convert to a native event.

Love it!

I guess this would also allow to add some deprecation messages when accessing event.originalEvent directly, as mentioned in the chapter's 3rd bullet point. With some shenanigans we could make sure that this deprecation is not triggered when accessing this through the normalizeEvent function.

This would also allow the transition to pass native events only (would be a breaking change) be painless, as addons can be refactored immediately to only use native event semantics, even when jQuery events are still passed for now.

@simonihmig
Copy link
Contributor Author

Just to give some more context, as this has been touched here: #294 (comment)...

This RFC is about providing an officially supported way to opt-out of jQuery. It is a non breaking change, and so far no deprecations are planned for this!

There will probably be a second follow up RFC, that is more aggressive (and thus maybe more controversial) about pushing away from jQuery, including deprecating jQuery coupled APIs like this.$(). Basically it would change the semantics from this one's "jQuery included by default, allow opt-out" to "removed by default, allow opt-in".

But that would be complimentary to this one and would obviously require the opt-out proposed here to be already possible, so it would basically be the second step (if adopted).

@benkingcode
Copy link

benkingcode commented Jan 13, 2018

I'm not sure enabled by default is the best approach. Also, I'm a bit confused, does this mean there would be two addons - one called something like ember-disable-jquery and one for @ember/jquery? I think it'd be much more elegant if jQuery was disabled by default and users could simply install @ember/jquery, and tools like ember-cli-update could do that automatically.

@simonihmig
Copy link
Contributor Author

does this mean there would be two addons - one called something like ember-disable-jquery

Managing the opt-out flag would be the responsibility of a dedicated addon, presumably ember-optional-features, which is not restricted to jQuery. See implementation outline

I think it'd be much more elegant if jQuery was disabled by default

I am also in favor of being more aggressive about moving away from jQuery at some point in time, but as I said that could be the subject of another follow up RFC. To really remove the existing jQuery support from ember-source (like this.$()) would be a breaking change, so can be done at Ember 4.0 at the earliest (with prior deprecation). Enabled by default is what we have today, and this RFC just opens the door to disable...

What we probably could do earlier is to opt-out by default in the default app blueprint, which would affect new apps, but would not break things for existing ones. But even that I think we should do carefully, when the most popular addons are known to work fine without jQuery, to not create a frustrating developer experience.

@rwjblue
Copy link
Member

rwjblue commented Jan 14, 2018

I think it'd be much more elegant if jQuery was disabled by default and users could simply install @ember/jquery, and tools like ember-cli-update could do that automatically.

I agree that is the longer term plan, but again this RFC is solely focused on allowing opting out to begin with. I believe as part of that plan, the default addon blueprint will begin testing without jQuery as well as with it and that will help make using Ember without jQuery easier and easier as addons update to absorb that change.

I absolutely agree with the goal of having ember new foo simply not use jQuery at all, we just need to move with care. 😸

@benkingcode
Copy link

By 'elegant' I meant it seems strange to me that a user would have to install something to remove something else. I agree that jQuery should be provided out of the box by default for now. But I think conceptually it doesn't make sense to have "lack of jQuery" being a new dependency.

@rwjblue
Copy link
Member

rwjblue commented Jan 14, 2018

By 'elegant' I meant it seems strange to me that a user would have to install something to remove something else.

Gotcha, sorry for misunderstanding!

But I think conceptually it doesn't make sense to have "lack of jQuery" being a new dependency.

Considering this RFC alone, I would completely agree with you. However, the actual intention here is to have a single @ember/optional-features addon that allows opting out of a number of currently defaulted features (application-template-wrapper and template-only-component-wrapper are the other two in addition to this one which would be jquery-integration). As features continue to be made optional (ultimately including being stripped from the build when not enabled) in Ember itself this list will grow even larger....

@simonihmig
Copy link
Contributor Author

I just added a new section with a possible solution to the jQuery.Event vs. native issue, based on @rwjblue's suggestion here!

Please review! 🧐

To solve this issue another addon `ember-jquery-legacy` will be introduced, which for now will only expose a single
`normalizeEvent` function. This function will accept a native event as well as a jQuery event (possibly distinguishing
between those two modes at build time, based on the jQuery integration flag), but will always return a native event
only.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it be necessary for ember-jquery-legacy to be a dependency or can it be just a devdependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will have to be a dependency (for an addon), as normalizeEvent needs to be imported in actual code, see the little code example! Only needed though if you currently require any jQuery specifics like originalEvent.

To encourage addon authors to refactor their jQuery coupled event code, the use of `jQuery.Event` specific APIs used for
jQuery events passed to component event handlers should be deprecated and a deprecation message be shown when accessing
them (e.g. `event.originalEvent`). Care must be taken though that this warning will not be issued when `normalizeEvent`
has to access `originalEvent`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This may require passing something that extends jquery.Event.

possible, like enabling tree shaking with the new [Module API](https://github.com/emberjs/rfcs/blob/master/text/0176-javascript-module-api.md),
moving code from core to addons (e.g. the [`Ember.String` deprecation](https://github.com/emberjs/rfcs/blob/master/text/0236-deprecation-ember-string.md))
or the ["Explode RFC"](https://github.com/emberjs/rfcs/blob/explode/text/0000-explode.md). In that regard removing the
dependency on jQuery is a rather low hanging fruit with an high impact.
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering the implementation complexity, describing this as "low hanging fruit" seems ridiculous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which implementation complexity are you referring to? For Ember itself, most of the hard work has already been done, what is proposed here should not be too hard to implement.

If you refer to user's apps, sure that might involve a lot of work to get rid of jQuery depending on the amount of jQuery code (so "your milage may vary"). However nobody forces you to do so, and for apps with less of an jQuery legacy, removing it could be a big and rather easy win!

@locks locks added this to In Progress in Deprecation Candidates Jan 18, 2018
@locks locks removed this from In Progress in Deprecation Candidates Jan 18, 2018
@rwjblue
Copy link
Member

rwjblue commented Jan 22, 2018

We discussed this at the core team meeting on 2018-01-19, and believe it is ready for final comment period.

Making it so...

@locks locks added the T-framework RFCs that impact the ember.js library label Feb 4, 2018
@rwjblue
Copy link
Member

rwjblue commented Feb 4, 2018

Thanks to all those that helped push this forward! There have been no new concerns brought up while this has been in the final comment period, and the core team is still very excited to move forward with it.

✂️

@rwjblue rwjblue merged commit 5b5d1d0 into emberjs:master Feb 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period T-framework RFCs that impact the ember.js library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants