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

Drop jQuery as a dependency #474

Closed
wants to merge 72 commits into from
Closed

Conversation

liudangyi
Copy link

As discussed in rails/rails#25208, web APIs in browsers have developed a lot in past few years, making it possible for us to build UJS without jQuery. This PR primitively refactors the whole UJS based on native APIs.

Browsers compatibility

According to liudangyi#1, the new UJS should support IE10+ and all other modern browsers.

Main changes

  1. Organize the source code as an NPM package using webpack, rather than a single monolithic file. Related PR: Add official support to npm #473.
  2. Use many ES6 features, with babel as precompiler.
  3. Use eslint instead of jshint.
  4. No jQuery. The detailed solution is available here: Analyze which part of jQuery does UJS depend on liudangyi/jquery-ujs#1.

API changes

  1. "overrides" feature is not implemented yet, because I can hardly find any possible scenario.
  2. "window.rails" instead of "$.rails".
  3. Event system. Since we are using native events (CustomEvent) now, there are some changes in binding / triggering an event if using jQuery. Some examples:
// Add an event listener using jQuery
// before
$(form).bind('ajax:success', function(e, data, status, xhr) {
  // code
})
// after
$(form).bind('ajax:success', function(e) {
  var data = e.originalEvent.detail[0]
  var status = e.originalEvent.detail[1]
  var xhr = e.originalEvent.detail[2]
  // code
})

// Manually trigger an event
// before
$(link).trigger('click')
// after
$(link)[0].click()
// before
$(form).trigger('submit')
// after, because el.submit() does't fire the submit event!
if ($(form)[0].dispatchEvent(new Event('submit', { bubbles: true, cancelable: true })) !== false) {
  $(form)[0].submit()
}

Existing issues

  1. (New name for jquery-ujs liudangyi/jquery-ujs#6) Naming. UJS should have a new name with no "jQuery". Any idea?
  2. (jQuery compatibility and enhancement liudangyi/jquery-ujs#5) As "event system" in last section, maybe we should add some helper functions for binding / triggering events. A potential solution (for jQuery users) exists in test/settings.js. But is it necessary or not?

liudangyi and others added 30 commits June 23, 2016 22:41
exec will replace rake itself!
See #2

Note: shotgun has a delay in handling HTTP request and may break the
test from time to time. So we call test:server instead of
test:reloadable in development.
Usage: npm run lint

See #3
Use different indentation for variable declarator.
Most tests pass, except for three override tests since we are using a
"static linking" provided by ES2015 now.

It'll be fixed in the following commits.
So that all utils have no dependency on others
The priority of this feature is not very high.
Fired when the ajax is stopped by "ajax:before" or "ajax:beforeSend".

Now a data-remote request would either ends with "ajax:complete" or
"ajax:stopped".
NOTE: File uploading support is removed in this commit, since we will
employ a new method (FormData) to handle ajax sooner.
@liudangyi
Copy link
Author

The code has been rewritten using CoffeeScript now. Could anyone help to review it?

@pixeltrix
Copy link

@liudangyi thanks your work on this - I'm not the biggest user of CoffeeScript so it's probably better that @javan is the final arbiter on that but it looks good to me. Also apologies on leading you astray with ES6 and WebPack - I think it's better in the end that we're consistent across all our JS projects.

@rafaelfranca is the plan to keep this code in the same repo or should we create a new repo for it? In some ways keeping it in the same repo makes sense - the legacy jquery-ujs would be on a separate branch and this new code would be our supported path going forward. My main concern is all of the existing links to this repo - if we rename it to rails-ujs how long does GitHub maintain redirects?

@rafaelfranca
Copy link
Member

@rafaelfranca is the plan to keep this code in the same repo or should we create a new repo for it? In some ways keeping it in the same repo makes sense - the legacy jquery-ujs would be on a separate branch and this new code would be our supported path going forward. My main concern is all of the existing links to this repo - if we rename it to rails-ujs how long does GitHub maintain redirects?

I could not find any information about redirects in the GitHub help apart from the note that if you create a repository with the same name the redirect will break.

I believe a new repository will be better for this case, there are still applications that will request jquery-ujs for a long time and having it as a branch here will make harder to deal with issues and pull requests since the reporter will have to point to the right branch.

@javan
Copy link

javan commented Aug 23, 2016

@liudangyi, at a glance it looks pretty good! I'll try to review more thoroughly soon. Have you tested it in an actual Rails app?

All, is the aim to exactly match the current behavior (but without jQuery) or should we take this opportunity review and maybe trim some fat?

@liudangyi
Copy link
Author

@javan I was trying to keep the same behavior as old jquery-ujs and all old tests are kept and passed (with necessary modification). But I don't think we must comply with the old behavior, since there are already some differences in events and ajax now.

@connorshea
Copy link

connorshea commented Aug 24, 2016

I don't mean to act a peanut gallery or anything, but I prefer plain JavaScript over CoffeeScript if only because the general pattern has been moving to either Vanilla JavaScript or ES6.

We've moved from CS to ES6 at GitLab 1, and Atom is slowly moving all their CoffeeScript over to ES6 [2]. There's a lot of momentum in moving from CoffeeScript to vanilla JavaScript (at least, vanilla once ES6 becomes supported by all major browsers in the future), and I wanted to at least mention it before we dig ourselves into a deeper hole.

Also worth noting that the CoffeeScript repository hasn't seen as much activity lately, and async/await support still isn't there.

Anecdotal evidence, but after moving to ES6 at GitLab we've seen more frontend developers contributing to the project. CS just isn't as friendly to frontend developers when compared to the vanilla/close-to-vanilla JavaScript that they're accustomed to.

Again, apologies for derailing this at all, and if there's a better place for me to open a discussion on this topic I'll gladly take it there.

[2]: atom/toggle-quotes#26 (comment) atom/atom@ab9a2a9

@gitmihalis
Copy link

Let me know if you'd like some help shoring this one up @liudangyi .

@guilleiguaran
Copy link
Member

@liudangyi is this ready for review?

@liudangyi
Copy link
Author

@guilleiguaran Yes.

source 'https://rubygems.org'

gem 'sinatra', '~> 1.0'
gem 'shotgun', :group => :reloadable
gem 'thin', :group => :reloadable
gem 'rake'
gem 'blade'
gem 'uglifier'
gem 'sprockets-commoner'
Copy link
Member

@guilleiguaran guilleiguaran Nov 15, 2016

Choose a reason for hiding this comment

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

I don't think this gem is being used anymore in the PR, is it?

Copy link
Author

Choose a reason for hiding this comment

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

You are right. My mistake.

Actually we are not using them at all.
@guilleiguaran
Copy link
Member

👍 on my side but would like to have the review of one of the maintainers of the library before of merging.

@rafaelfranca
Copy link
Member

I still think this should be in another repository. jquery_ujs should be still here for people that use jquery and want to keep using. Also issues for jquery_ujs not necessarily will be issues in non-jquery_ujs, so even for issue triage that will be better.

@guilleiguaran
Copy link
Member

@rafaelfranca agree totally, let's do it in that way

guilleiguaran added a commit to rails/rails-ujs that referenced this pull request Nov 17, 2016
@guilleiguaran
Copy link
Member

guilleiguaran commented Nov 17, 2016

Ok, now the code of this PR is living under https://github.com/rails/rails-ujs, feel free to open PRs with improvements or open a issue in the new repo if you found any problem related to new implementation.

@jakeNiemiec
Copy link
Member

@guilleiguaran Out of curiosity, what happened to rails-ujs?

@guilleiguaran
Copy link
Member

rails-ujs was merged inside of rails core: https://github.com/rails/rails/tree/master/actionview/app/assets/javascripts

@askehansen
Copy link

askehansen commented Jun 8, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet