Drop jQuery as a dependency #27113

guilleiguaran · 2016-11-19T23:24:45Z

As discussed in #25208 we have decided to remove jQuery from default stack and use a vanilla version of the ujs driver named rails-ujs instead.

This Pull Request remove jquery-rails from new applications and provides rails-ujs through Action View (to do this I had to convert ActionView::Railtie into an Engine).

The new rails-ujs was developed by @liudangyi as part of Google Summer of Code, all credits for this goes to him and to his mentor @pixeltrix 👏 👏 👏 👏 👏

jeremy · 2016-11-20T02:51:03Z

actionview/lib/action_view/engine.rb

+
+module ActionView
+  # = Action View Engine
+  class Railtie < Rails::Engine # :nodoc:


How about leaving this in lib/action_view/railtie.rb? The fact that its superclass is now Rails::Engine doesn't mean it needs to move.

jeremy · 2016-11-20T03:50:29Z

railties/lib/rails/generators/rails/app/templates/app/assets/javascripts/application.js.tt

 //= require <%= options[:javascript] %>
 //= require <%= options[:javascript] %>_ujs
+<% else -%>
+//= require rails-ujs
+<% end -%>


Perhaps we should always use rails-ujs and eliminate *_ujs

agree, I was thinking exactly the same

kaspth · 2016-11-20T13:35:02Z

railties/lib/rails/generators/app_base.rb

-                                       "Use #{options[:javascript]} as the JavaScript library")
+          if options[:javascript]
+            gems << GemfileEntry.version("#{options[:javascript]}-rails", nil,
+             "Use #{options[:javascript]} as the JavaScript library")


I think this line needs another space to indent it fully 😁

thanks, updated!!

kaspth · 2016-11-20T13:48:33Z

Congrats @liudangyi, nice work! ❤️

prathamesh-sonpatki · 2016-11-20T15:20:47Z

This needs changelog as well 😃

guilleiguaran · 2016-11-20T17:16:28Z

CHANGELOG entries added 😊

javan · 2016-11-20T22:29:30Z

Does the compiled rails-ujs.js file need to be added here? I'd prefer that we A) fold all of rails-ujs into Rails and compile it on release like with do for Action Cable or B) make rails-ujs a gem and add its dist/ path to the asset load path.

guilleiguaran · 2016-11-20T22:48:37Z

@javan the different options were discussed in #25208 (comment), B) was discarded but maybe A) is preferred more than the current approach /cc @dhh @rafaelfranca

In my opinion is a better idea to keep the source code out of Rails repo because of maintenance (typically the maintainers of js integration libraries are different than the maintainers of Rails and that's why we have a "Javascript" collaborators group on Github) and distribution (we don't expect to sync releases of this library with the releases of Action View like we do for Action Cable)

Personally I'll prefer to keep the current approach or B) making it a gem.

matthewd · 2016-11-20T23:46:21Z

I think it should be a gem that is explicitly named in the Gemfile.

For the relevant helpers to work, actionview needs a UJS implementation to be present -- but that could be this new one as supplied by us, it could be jquery-rails, or it could be managed by another asset manager.

We have plenty of precedent for features that only work, or work much better, if an additional optional gem is present -- from database adapters, to bcrypt passwords. And the "U" in "UJS" is supposed to promise that the helpers' HTML output will still get the job done even with no JS available. (After all, even if we ship it, we can't guarantee it's included on a given page.)

Mostly, I'm worried that having two UJS assets floating around inside an application risks confusing conflicts, so it seems better that this new one will Properly Go Away if it's not being used. (Particularly given that while we're not yet strongly committed to the idea, and a good way away from requiring it, it seems likely we're going to end up with a 3rd party asset manager as the default happy path in the not-too-distant future.)

I agree with @javan that it should at least be part of the build process, not a manually copied file, if we do choose to ship it built-in. And if we're bundling the compiled result into the AV gem, then we do seem to intend to sync releases of the library -- in the most physical way possible. 😕

javan · 2016-11-21T00:02:23Z

We currently have two "systems" for incorporating JavaScript:

Compile on release (Action Cable)
External gems (Turbolinks, jquery-ujs, jquery-rails)

I'd rather not introduce a third, especially if it's copy+paste

rafaelfranca · 2016-11-21T00:26:09Z

Thinking about the release process I still prefer it to be a gem and for
those who don't want the gem we have a npm package.
On Sun, Nov 20, 2016 at 7:02 PM Javan Makhmali [email protected]
wrote:

We currently have two "systems" for incorporating JavaScript:

Compile on release (Action Cable)

External gems (Turbolinks, jquery-ujs, jquery-rails)

I'd rather not introduce a third, especially if it's copy+paste

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#27113 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAC66IYWjiSh_Qs7a10N4MZAlyIG4VVVks5rAN-RgaJpZM4K3Zlz
.

liudangyi · 2016-11-21T09:07:22Z

I would suggest to make it a separate gem. An additional line in Gemfile won't be a trouble for users but adds flexibility for customization and release process.

dhh · 2016-11-21T10:51:15Z

I'm curious as to why we think this package is going to need a higher
release churn than Rails itself? UJS isn't really something that's been
seeing a lot of additional features being added. So it's a stable package
that just provides our baseline. Given that, I think having it as part of
Action View, like we do with Action Cable, is the better way to go.

But, again, if Rafael is doing the release work, and he prefers a separate
gem, that's OK with me too.

On Mon, Nov 21, 2016 at 10:07 AM, Dangyi Liu [email protected]
wrote:

I would suggest to make it a separate gem. An additional line in Gemfile
won't be a trouble for users but adds flexibility for customization and
release process.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27113 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAKtSiu-io0sHgj-Kc9qfsHKPf6dD6lks5rAV9ngaJpZM4K3Zlz
.

guilleiguaran · 2016-11-21T19:44:35Z

Updated and now jquery-ujs gem is added to Gemfile.

…ter instead

rafaelfranca

If we claim to this library to be ubstrusive why even bother to allow people to chose another one? I'm more inclined to just remove support to other javascript drivers in the generators

rafaelfranca · 2016-11-22T19:13:33Z

railties/lib/rails/generators/app_base.rb

@@ -328,8 +328,13 @@ def javascript_gemfile_entry
          gems = [javascript_runtime_gemfile_entry]
          gems << coffee_gemfile_entry unless options[:skip_coffee]

-          gems << GemfileEntry.version("#{options[:javascript]}-rails", nil,
-                                       "Use #{options[:javascript]} as the JavaScript library")
+          if options[:javascript]


Why doing this here and not using the already existing default option in Thor?

Nevermind got it

rafaelfranca · 2016-11-22T19:14:40Z

railties/lib/rails/generators/rails/app/templates/app/assets/javascripts/application.js.tt

 //= require <%= options[:javascript] %>
-//= require <%= options[:javascript] %>_ujs
+<% end -%>


Would not this add both in case people have Jquery-ujs?

For users passing the -j jquery option this file will be like:

//= require jquery //= require rails-ujs

So we are making rails-ujs the default even for jQuery users and not allowing people to chose another driver

oh. I miss read it.

guilleiguaran · 2016-11-22T19:24:07Z

@rafaelfranca yup, we are doing exactly that in generators, even the jQuery users will get rails-ujs instead of jquery_ujs by default

rafaelfranca · 2016-11-22T19:41:50Z

👍

dhh · 2016-11-23T11:02:02Z

@rafaelfranca I think that's a strong argument for just including it in Action View. We don't need an adapter for this. This is UJS support to make the Rails helpers work and they just rely on standard JS. No reason to have a jQuery or whatever version of them imo.

guilleiguaran · 2016-11-23T13:55:02Z

@dhh agree on that

@rafaelfranca wdyt?

rafaelfranca · 2016-11-23T23:38:18Z

This will make the release process really hard because we will have to clone two repositories build the dist file etc. We can make it simpler though if we move all code to inside action view. That way we can release it in the same way we do release actioncable. But doing this we will lose the separated issue tracker and will complicate our test matrix that is already slow, and complicated.

If we are going to move the entire code to action view I'm positive about it, but there are those drawbacks that I pointed.

Rails dropped jQuery as a dependency back in rails/rails#27113. That commit removed 'jquery-ujs' from the application.js and replaced it with 'rails-ujs'. Since we aren't using 'jquery-ujs', we don't need the 'jquery-rails' gem. As for the test JS, `$.fx.off = true` shouldn't be necessary if we aren't using jQuery, and `$.ajaxSetup({ async: false });` seems to have been for Akephalos, which we no longer use.

maclover7 added the asset pipeline label Nov 20, 2016

guilleiguaran added actionview railties labels Nov 20, 2016

guilleiguaran mentioned this pull request Nov 20, 2016

Drop jQuery as a dependency #25208

Closed

guilleiguaran force-pushed the remove-jquery branch from 7967631 to d6262d7 Compare November 20, 2016 02:42

jeremy reviewed Nov 20, 2016

View reviewed changes

guilleiguaran force-pushed the remove-jquery branch from d6262d7 to 5a7bed1 Compare November 20, 2016 03:30

jeremy reviewed Nov 20, 2016

View reviewed changes

guilleiguaran force-pushed the remove-jquery branch from 5a7bed1 to 7184289 Compare November 20, 2016 04:46

kaspth reviewed Nov 20, 2016

View reviewed changes

guilleiguaran force-pushed the remove-jquery branch from 7184289 to 84f8cb5 Compare November 20, 2016 16:57

guilleiguaran assigned dhh Nov 20, 2016

guilleiguaran added this to the 5.1.0 milestone Nov 21, 2016

guilleiguaran force-pushed the remove-jquery branch from dd2e5d8 to edec43d Compare November 21, 2016 19:30

guilleiguaran added 3 commits November 21, 2016 18:58

Don't add jQuery by default in new apps and use rails-ujs as UJS adap…

a865e3b

…ter instead

Add CHANGELOG entry

82d683a

Add rails-ujs as dependency in the rails Gemfile

277b5f9

guilleiguaran force-pushed the remove-jquery branch from 3750e8d to 277b5f9 Compare November 21, 2016 23:58

guilleiguaran merged commit 49aa974 into master Nov 22, 2016

rafaelfranca reviewed Nov 22, 2016

View reviewed changes

rafaelfranca deleted the remove-jquery branch November 22, 2016 19:41

composerinteralia mentioned this pull request Oct 26, 2018

Drop jquery-rails gem thoughtbot/suspenders#948

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop jQuery as a dependency #27113

Drop jQuery as a dependency #27113

guilleiguaran commented Nov 19, 2016 •

edited

Loading

jeremy Nov 20, 2016

guilleiguaran Nov 20, 2016

jeremy Nov 20, 2016

guilleiguaran Nov 20, 2016

kaspth Nov 20, 2016

guilleiguaran Nov 20, 2016

kaspth commented Nov 20, 2016

prathamesh-sonpatki commented Nov 20, 2016

guilleiguaran commented Nov 20, 2016

javan commented Nov 20, 2016

guilleiguaran commented Nov 20, 2016 •

edited

Loading

matthewd commented Nov 20, 2016

javan commented Nov 21, 2016

rafaelfranca commented Nov 21, 2016

liudangyi commented Nov 21, 2016

dhh commented Nov 21, 2016

guilleiguaran commented Nov 21, 2016

rafaelfranca left a comment

rafaelfranca Nov 22, 2016

rafaelfranca Nov 22, 2016

rafaelfranca Nov 22, 2016

guilleiguaran Nov 22, 2016

rafaelfranca Nov 22, 2016

guilleiguaran commented Nov 22, 2016

rafaelfranca commented Nov 22, 2016

dhh commented Nov 23, 2016

guilleiguaran commented Nov 23, 2016

rafaelfranca commented Nov 23, 2016

Drop jQuery as a dependency #27113

Drop jQuery as a dependency #27113

Conversation

guilleiguaran commented Nov 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspth commented Nov 20, 2016

prathamesh-sonpatki commented Nov 20, 2016

guilleiguaran commented Nov 20, 2016

javan commented Nov 20, 2016

guilleiguaran commented Nov 20, 2016 • edited Loading

matthewd commented Nov 20, 2016

javan commented Nov 21, 2016

rafaelfranca commented Nov 21, 2016

liudangyi commented Nov 21, 2016

dhh commented Nov 21, 2016

guilleiguaran commented Nov 21, 2016

rafaelfranca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilleiguaran commented Nov 22, 2016

rafaelfranca commented Nov 22, 2016

dhh commented Nov 23, 2016

guilleiguaran commented Nov 23, 2016

rafaelfranca commented Nov 23, 2016

guilleiguaran commented Nov 19, 2016 •

edited

Loading

guilleiguaran commented Nov 20, 2016 •

edited

Loading