SolutionGuidance / psm

Welcome to the Medicare/Medicaid Provider Enrollment Screening Portal
http://projectpsm.org/
Other
26 stars 20 forks source link

Replace deprecated jQuery `.live(events, handler)` in JS code #582

Open PaulMorris opened 6 years ago

PaulMorris commented 6 years ago

As @jasonaowen notes:

apparently .live(events, handler) was deprecated in jQuery 1.7 (we use 1.7.1), and should be replaced with .on(events, handler); .click(handler) is a convenience method for .on('click', handler).

We should replace occurrences of .live(events, handler) in our JS code with non-deprecated methods.

PaulMorris commented 6 years ago

Since we're going to be making these changes, another possibility would be to just use the newer native DOM methods for element selection and click handling (like querySelector, querySelectorAll, and addEventListener). This would reduce our dependency on jQuery so that at some point we might be able to drop it.

Basically, over the years new native DOM methods have caught up to the jQuery equivalents. See for example: http://youmightnotneedjquery.com/ In particular for this issue: http://youmightnotneedjquery.com/#contains_selector http://youmightnotneedjquery.com/#on

jasonaowen commented 6 years ago

That sounds like a great plan!

PaulMorris commented 6 years ago

I did some preliminary work on this, and it's a little more involved than I thought to use the native methods.

First, in $('.selector').on('click', function... if the selector isn't found then nothing happens. Whereas with document.querySelectorAll('.selector').addEventListener('click', function... you get null when the selector isn't found, then an error is thrown when you try to call a method on null, and the rest of the JS past that point isn't run. So I wrote a little helper function to handle this:

function addListener (selector, eventType, listener) {
  var elements = document.querySelectorAll(selector);
  if (elements.length > 0) {
    // the next line is done this way for IE compatibility
    Array.prototype.forEach.call(elements, function (element) {
      element.addEventListener(eventType, listener);
    });
  }
}
addListener('.selector', 'click', function...

And using regex in a python script I was able to change to using this helper function instead of $(...).live('click', function...

(Here's the regex r"\$\((.*?(?:\'|\"))\)\s*\.live\(\s*'click'" and the substitution string r"addListener(\g<1>, 'click'" for the record.)

Second, there are a handful of jQuery-specific selectors that include things like input:radio, input:checkbox, or :not(".selectAll") that don't work with the native methods. So those we could just leave those as jQuery.

Third, the jQuery-ism $(this) inside event listener functions doesn't work if the listener was attached using native methods and not jQuery. And there seem to be about a third of these functions that use $(this) that would be stuck with the jQuery approach without further refactoring. Leaving us in a half-in / half-out situation.

So, this turns out to be more than a quick and clean find/replace operation. For now I think we should just update the jQuery syntax (from .live to .on and/or .click). Then at some point if we want to work on removing the jQuery dependency we can go further down this other road I've started exploring here.

@jasonaowen Thoughts?

jasonaowen commented 6 years ago

It sounds like removing jQuery as a dependency is not very realistic a goal right now, and I'm fine with that. Removing barriers to upgrading it, such as by replacing the deprecated jQuery API calls this issue refers to with calls to non-deprecated jQuery APIs, is still a useful first step.

I expect that the process of fixing the deprecated method calls and incrementally upgrading jQuery towards a more modern version would have us touch a lot of the JavaScript in the project, which I've found to be a very useful way to learn a codebase. Once we've done those fixes and upgrades, I think we'll have a better understanding of how useful jQuery is as a dependency, how hard it would be to remove it, and we'd be able to make better decisions around it.

I don't think I completely followed all the nuances of the native method replacements, but I think I got enough to suggest that our approach should be that we replace with native DOM API calls when it's a clear and easy win that doesn't change the behavior, and use newer jQuery API calls otherwise. How does that sound, @PaulMorris?