search

payolapayments / payola

Drop-in Rails engine for accepting payments with Stripe
http://www.payola.io
Other
818 stars 154 forks source link

Turbolinks Support / Are the recent JS changes to accomodate turbolinks necessary? #199

Closed nfm closed 6 years ago

nfm commented 8 years ago

Payola's javascripts were updated to make them more Turbolinks friendly in bfeaa83 and 2655313.

A side-effect of this change is that if the Payola buttons aren't in the page on document.ready(), click events aren't bound to them. For example, if the page loads, and the Payola subscription checkout button is later displayed in a modal, the click handler for that button will never be bound to it.

I'm not super familiar with turbolinks, but it looks to me like the changes might not have been necessary. Previously, Payola was using $(document).on('click', someSelector, someCallback) style event delegation, and that code was invoked once on document.ready(). AFAIK turbolinks doesn't run callbacks for document.ready() when it loads new content into the page, so my understanding is that the event delegator ($(document).on('click', someSelector, someCallback)) would only be called once.

I might be missing something obvious though. @spdawson, could you chime in?

Can we figure out a way to make this work for all use cases - vanilla, Turbolinks, and non-Turbolinks with dynamically loaded Payola buttons?

eliotsykes commented 8 years ago

@nfm Thank you for bringing this up.

I definitely could be missing something here, though right now, yes I'd expect the original approach would have worked with Turbolinks Classic, New Turbolinks, and non-Turbolinks apps.

Some abbreviated code changes to aid this discussion:

// Original approach
// - Q. Works with non-Turbolinks?
// - A. Yes
//
// - Q. Turbolinks Classic already worked with this?
// - A. Unclear, possibly. Could the event handler get attached multiple times?
//        Won't work if the payola js script tag is not present application-wide, say
//         if payola's js is excluded from application.js, and payola's js is only added
//         to checkout pages <head>. The `ready` event won't fire and initialize
//         will never be called if those checkout pages are loaded via Turbolinks.
//
// - Q. Would this work with New Turbolinks (Rails 5)?
// - A. If it works with Turbolinks Classic, yes.
//
// - Q. Works with checkout button inserted after delay (e.g. in a modal)?
// - A. Yes
var PayolaCheckout = {
  initialize: function() {
    $(document).on('click', '.payola-checkout-button', function(e) { ... }
  }
  ...
}
...
$(function() { PayolaCheckout.initialize(); });
// New approach
// - Q. Works with non-Turbolinks?
// - A. Yes
//
// - Q. Turbolinks Classic works with this?
// - A. Yes
//
// - Q. Would this work with New Turbolinks (Rails 5)?
// - A. No, 'page:change' event no longer exists
//
// - Q. Works with checkout button inserted after delay (e.g. in a modal)?
// - A. No
var PayolaCheckout = {
  initialize: function() {
    $('.payola-checkout-button').on('click', function(e) { ... }
  }
  ...
}
...
if ('undefined' !== typeof Turbolinks) {
    $(document).on('page:change', PayolaCheckout.initialize);
} else {
    $(document).ready(PayolaCheckout.initialize);
}

(summarized from commits https://github.com/peterkeen/payola/commit/bfeaa83a01f072bf29419dfe5745e8f834e5b4ce and https://github.com/peterkeen/payola/commit/2655313d5931ea3ce4e72916966ab5d1aa25653f on file app/assets/javascripts/payola/checkout_button.js)

It'd be good to get some more certainty in the answers to the questions asked above to help decide how to move forward.

Also, at time of writing, the README isn't detailed enough where it mentions Turbolinks support. It is at least partially supported with Turbolinks Classic as long as you're not loading checkout buttons or other Payola elements after the page:change event.

eliotsykes commented 8 years ago

Possible solution?

// Use namespace events to prevent duplicate handlers
var PayolaCheckout = {
  initialize: function() {
    $(document)
      .off('click.payola-checkout-button') // remove any previous handler
      .on('click.payola-checkout-button', '.payola-checkout-button', function(e) { ... }
  }
  ...
}
PayolaCheckout.initialize();

Event namespaces: https://api.jquery.com/on/#event-names

eliotsykes commented 8 years ago

One way to do Turbolinks compatability: https://github.com/DavyJonesLocker/client_side_validations/commit/15e055e95afc213cbc57f274bcad846c43225c45

eliotsykes commented 8 years ago

In regards to this possible change for detecting payola-related JS events:

// Use namespace events to prevent duplicate handlers
var PayolaCheckout = {
  initialize: function() {
    $(document)
      .off('click.payola-checkout-button') // remove any previous handler
      .on('click.payola-checkout-button', '.payola-checkout-button', function(e) { ... }
  }
  ...
}
PayolaCheckout.initialize();

As long as .on(...) is called on the document and a selector argument is given as the 2nd argument, e.g. .payola-payment-form in $(document).on('submit', '.payola-payment-form', ...) then the call does not need to wait for the document to finish loading:

The document element is available in the head of the document before loading any other HTML, so it is safe to attach events there without waiting for the document to be ready.

Source: https://api.jquery.com/on/#direct-and-delegated-events

The above change has all of the following features, some of which have not been available in any previous approaches:

The only disadvantage I can come up with is that multiple event handlers are attached to the document and some of them will be redundant. This was how Payola originally worked before the Turbolinks-related changes in https://github.com/peterkeen/payola/commit/bfeaa83a01f072bf29419dfe5745e8f834e5b4ce and https://github.com/peterkeen/payola/commit/2655313d5931ea3ce4e72916966ab5d1aa25653f and did not cause problems. However, its probably a good idea to nudge developers to only load the payola-*.js scripts that are relevant to their app rather than load all of them so clients are not forced to download more data than needed. By only loading the relevant payola-*.js script(s), redundant event handlers will not be added to the document.

nfm commented 8 years ago

@eliotsykes Thanks heaps for taking a look at this. I reckon your proposed solution is the most appropriate. Binding events to document, and unbinding first to avoid duplicated handlers is always safe, and is nice because it's not coupled directly to Turbolinks.

I think the issue of adding redundant event bindings is minor, and probably best solved in documentation.

nfm commented 8 years ago

I also just saw the README explicitly says Turbolinks is not supported, which is outdated info.

eliotsykes commented 8 years ago

Thanks for the feedback @nfm, very helpful. I'll plan now to introduce the above solution.

eliotsykes commented 8 years ago

See #254