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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop jQuery as a dependency #27113

Merged
merged 3 commits into from Nov 22, 2016
Merged

Drop jQuery as a dependency #27113

merged 3 commits into from Nov 22, 2016

Conversation

@guilleiguaran
Copy link
Member

@guilleiguaran guilleiguaran commented Nov 19, 2016

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 馃憦 馃憦 馃憦 馃憦 馃憦

actionview/lib/action_view/engine.rb Outdated

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

This comment has been minimized.

@jeremy

jeremy Nov 20, 2016
Member

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.

This comment has been minimized.

@guilleiguaran

guilleiguaran Nov 20, 2016
Author Member

Done!!!

@guilleiguaran guilleiguaran force-pushed the remove-jquery branch Nov 20, 2016
railties/lib/rails/generators/rails/app/templates/app/assets/javascripts/application.js.tt Outdated
//= require <%= options[:javascript] %>
//= require <%= options[:javascript] %>_ujs
<% else -%>
//= require rails-ujs
<% end -%>

This comment has been minimized.

@jeremy

jeremy Nov 20, 2016
Member

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

This comment has been minimized.

@guilleiguaran

guilleiguaran Nov 20, 2016
Author Member

agree, I was thinking exactly the same

@guilleiguaran guilleiguaran force-pushed the remove-jquery branch Nov 20, 2016
railties/lib/rails/generators/app_base.rb Outdated
"Use #{options[:javascript]} as the JavaScript library")
if options[:javascript]
gems << GemfileEntry.version("#{options[:javascript]}-rails", nil,
"Use #{options[:javascript]} as the JavaScript library")

This comment has been minimized.

@kaspth

kaspth Nov 20, 2016
Member

I think this line needs another space to indent it fully 馃榿

This comment has been minimized.

@guilleiguaran

guilleiguaran Nov 20, 2016
Author Member

thanks, updated!!

@kaspth
Copy link
Member

@kaspth kaspth commented Nov 20, 2016

Congrats @liudangyi, nice work! 鉂わ笍

@prathamesh-sonpatki
Copy link
Member

@prathamesh-sonpatki prathamesh-sonpatki commented Nov 20, 2016

This needs changelog as well 馃槂

@guilleiguaran
Copy link
Member Author

@guilleiguaran guilleiguaran commented Nov 20, 2016

CHANGELOG entries added 馃槉

@javan
Copy link
Member

@javan javan commented Nov 20, 2016

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
Copy link
Member Author

@guilleiguaran guilleiguaran commented Nov 20, 2016

@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
Copy link
Member

@matthewd matthewd commented Nov 20, 2016

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
Copy link
Member

@javan javan commented Nov 21, 2016

We currently have two "systems" for incorporating JavaScript:

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

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

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Nov 21, 2016

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 notifications@github.com
wrote:

We currently have two "systems" for incorporating JavaScript:

  1. Compile on release (Action Cable)
  2. 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
.

@guilleiguaran guilleiguaran added this to the 5.1.0 milestone Nov 21, 2016
@liudangyi
Copy link

@liudangyi liudangyi commented Nov 21, 2016

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
Copy link
Member

@dhh dhh commented Nov 21, 2016

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 notifications@github.com
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 guilleiguaran force-pushed the remove-jquery branch from dd2e5d8 to edec43d Nov 21, 2016
@guilleiguaran
Copy link
Member Author

@guilleiguaran guilleiguaran commented Nov 21, 2016

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

@guilleiguaran guilleiguaran force-pushed the remove-jquery branch from 3750e8d to 277b5f9 Nov 21, 2016
@guilleiguaran guilleiguaran merged commit 49aa974 into master Nov 22, 2016
3 checks passed
3 checks passed
codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Copy link
Member

@rafaelfranca rafaelfranca left a comment

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

@@ -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]

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 22, 2016
Member

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

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 22, 2016
Member

Nevermind got it

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

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 22, 2016
Member

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

This comment has been minimized.

@guilleiguaran

guilleiguaran Nov 22, 2016
Author Member

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

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 22, 2016
Member

oh. I miss read it.

@guilleiguaran
Copy link
Member Author

@guilleiguaran guilleiguaran commented Nov 22, 2016

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

@rafaelfranca rafaelfranca deleted the remove-jquery branch Nov 22, 2016
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Nov 22, 2016

馃憤

@dhh
Copy link
Member

@dhh dhh commented Nov 23, 2016

@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
Copy link
Member Author

@guilleiguaran guilleiguaran commented Nov 23, 2016

@dhh agree on that

@rafaelfranca wdyt?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Nov 23, 2016

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.

composerinteralia added a commit to thoughtbot/suspenders that referenced this pull request Oct 26, 2018
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.
composerinteralia added a commit to thoughtbot/suspenders that referenced this pull request Oct 30, 2018
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can鈥檛 perform that action at this time.