Use property definition to normalize read-only event target and which

[ddayguerrero]

Fixes #37

The following snippets demonstrate the difference between property assignment and definition:

Property assignment (Issue):

Property definition (Fix):

Changes

• Use property definition instead of property assignment to normalize UI keyboard event properties (i.e. target and which) across major desktop browsers.

Why introduce these changes?

• Allows Payform to be vendored with using ES6 imports
• Solve TypeErrors which were being raised (see #37 for more details).

How is this achieved?

• Normalize target and which KeyboardEvent properties definition through the Object.defineProperty() static method.

• Since we want to maintain our compatibility with IE8, we create a "synthetic" event object with the necessary properties used throughout Payform.

Additional Information

1. Section 10.2.1 of the official ES6 specification states that:

   Module code is always strict mode code.

This prevents assignment of read-only properties.

2. How is property assignment different from property definition? Why does the former throws errors?

According to Dr. Axel Rauschmayer, there are noticeable differences between property definition and assignment. Specifically, when dealing with read-only properties such as target:

Read-only properties in prototypes prevent assignment, but not definition.

[jondavidjohn]

Good work here in explaining the issue! Unfortunately, it looks like Object.defineProperty is not available in legacy browsers like IE8 (for which this code tries to support).

[ddayguerrero]

Ah, right! It only work on DOM objects. What do you think about adding a IE8 polyfill?

[jondavidjohn]

Not sure all this is worth it, I'd say the simpler approach would be to return a synthetic event object from _eventNormalize

[ddayguerrero]

I made some changes and attempted to create a "synthetic event" object in _eventNormalize. This works in IE8, however as you can see I am cherrypicking properties that are used in the listeners. See the updated demo.

Here are some of my other attempts:

• Using CustomEvent but no IE8 support.
• Using var newEvt = new e.constructor(e.type, e) but no luck in reassigning properties due to read-only.

In the process of seeing how React uses their own SyntheticEvents but it seems that IE8 is not supported.

[jondavidjohn]

Sure! I think it's fine to just mock the properties we use, as long as the event acts as we want it to. We're not exposing this fake event to the outside 👍

[ddayguerrero]

I've looked into how React normalizes some of its properties and unfortunately it does not support IE8. SyntheticEvent uses Object.create() to specify properties such as target.

Having said, I've tested latest browsers and IE8+. Everything is stable and tests are still passing. As you have mentioned, it is important that we do not leak the visibility of this synthetic event. Shall we need to handle specific event properties in the future, we can simple assign them in _eventNormalize.

[ddayguerrero]

I've done some browsers tests. I made sure validation and formatting still works and that no TypeErrors are thrown.

Desktop 🖥:

• Latest Chrome ✅
• Latest Firefox ✅
• Latest Safari ✅
• Latest Edge ✅
• IE8 ✅
• IE10 ✅

Mobile 📱:

• iOS Safari and Chrome ✅
• Android Chrome and Firefox ✅

[arthurgouveia]

Code looks good to me and also was able to see the problem was solved

[jondavidjohn]

@ddayguerrero Thanks for this, I changed the target branch to develop and looks like there is a conflict that needs to be resolved.

(and thanks @arthurgouveia for the second set of eyes!)

[ddayguerrero]

Thanks for taking your time @jondavidjohn, I solved the conflicts and did a manual sanity test on changes with develop on this sample.

Let me know if anything!