Adds support for detaching events from input fields

[sergiocruz]

Detaching formatting helpers

This PR references issue #61 where I asked if there was a proper way to detach events from elements once we used the formatting helpers.

I asked that question because I am using payform in a Single Page Application environment where it's extra important to be able to detach event listeners from input elements before removing them from the DOM to avoid memory leaks.

API Changes

I created 4 new methods to make this possible (on both main plugin & jQuery implementation):

• payform.detachCardNumberInput
• payform.detachExpiryInput
• payform.detachCvcInput
• payform.detachNumericInput

Approach

In order to property detach events from input elements, we have to keep a reference of the original function handler that was used to attach the event. To achieve this, I created a new variable that is called eventList. The eventList variable contains an object that holds all of our four input types (cvcInput, expiryInput, cardNumberInput, numericInput).

These input types hold arrays of objects containing the event names and their normalized handlers. I had to normalize the handlers here to ensure we kept the same reference of the handler function to be able to then use that same reference to remove the event listeners from elements.

On the methods formatting methods now, instead of making multiple calls to our _on() method, we run a for loop on the respective event types, therefore attaching the same events as before with their respective handlers, in the same order as before.

To achieve the actual detachment of events, I created a new internal method called _off(). I followed the jQuery fashion a la .on() and .off() methods for this.

Questions?

Please don't hesitate to add any questions, comments on concerns to this PR as I will be happy to address them.

Also feel free to add suggestions to different approaches I should have used instead as I will be happy to try them out as well.

Thanks you all :)

[ddayguerrero]

What do you think about refactoring the loop for attaching the events via _on?

[sergiocruz]

@ddayguerrero sure thing! Can you clarify a little further on how you'd like me to refactor?

[ddayguerrero]

Sure! 😊 I considered this request because I noticed a pattern in all the for loops defined.

I suggest predefining all inputs (e.g. expiryInput, cardNumerInput, etc.) inside an array or JSON object. Then we can iterate through each type and define the function which calls the loop and attaches the event listeners.

Let me know if this makes sense!

[sergiocruz]

@ddayguerrero hey I just did a variation of what you suggested. Let me know what you think now

[ddayguerrero]

Thanks for the good work; this looks promising! 👍 I'm wondering if it worth renaming the API as the event attachment is implied when using the {field}Input(input) function.

 payform.cvcInput(input);
 payform.detachCvcInput(input);

to

 payform.attachCvcInput(input);
 payform.detachCvcInput(input);

May be far fetched as it will definitely disrupt our users and force us to create a more significant release version. I'll wait for @jondavidjohn input on this solution as a whole.

[sergiocruz]

Yeah, I was a little weary on releasing a breaking change to the API given how many people currently use this library.

My initial idea was for it to be a minor release where we introduce new methods to the API but no breaking changes. But I'll definitely also defer to @jondavidjohn here and let him make the final decision around that.

It would be a quick change on my end, so if that's what we decide to do, I can amend the PR very swiftly.

[jondavidjohn]

Yeah, let's hold off any any breaking changes for now, this should definitely be a minor bump, but I don't think a major bump is worth it simply for semantics.

Good thought on making them consistent, keep that in mind if we ever have to force a breaking change to circle back. 👍

[sergiocruz]

Good deal @jondavidjohn! Do you see anything else we should do before merging this in?

[jondavidjohn]

👏 Thanks for the docs additions as well.

[ddayguerrero]

I did one last check and ran local tests, everything looks good to me!

[ddayguerrero]

@jondavidjohn We've made all the necessary changes as per the contribution guidelines. Would you mind doing a last check before I publish to npm? 🙂

[sergiocruz]

Hey @ddayguerrero or @jondavidjohn I see a 1.4.0 release here on GitHub but not on npm. Is that on purpose? I'd like to implement this functionality into my app.

Thank you!

[jondavidjohn]

@ddayguerrero Yeah, you're good to go, no need to get my approval 👍

[ddayguerrero]

@jondavidjohn perfect, thanks! 🎉
@sergiocruz expect an update by the end of the day :)

[sergiocruz]

Thank you @ddayguerrero, I'll be looking forward to seeing it!