Fix potential compatibility issue with other libraries

[laysent]

trafficstars

Hi, there are two commits in this PR, aimed to fix potential compatibility issue with other libraries. (i.e. the code was correct, just want to make sure it will work when environment has some strange implementation)

1. First commit is to improve Symbol implementation check. I saw #423 has been merged already. Hopefully it can be released soon. This is only a tiny improvment of #423, and might seem a bit unnecessary. Here is the reason: Our code might be running on client's website where environment is not under control. We have experienced client who changed Array.prototype.forEach to act different behavior. Thus, theoratically, it's possible that Symbol.for is truthy, but just not a function, which will still cause issue. Just want to be cautious here, though I know that will not happen 99.999999% of the time.

2. Second commit is to remove .bind usage for partial function. I noticed that old libraries before ESCMAScript 5 might define .bind api differently. For example: mootools So far from what I know, it should be fine to use .bind to bind this context. But using .bind to add some parameters might not be supported by those old libraries. Thus, I have changed it to use rest parameters instead.

[developit]

Sorry for the super long delay, I was taking a break from compat issues.

I need to check what the performance and size hit are for dropping .bind() - TBH, I'm not sure mootools is enough justification for avoiding .bind. That's sortof a slippery slope where no builtins can be relied on ([].slice, etc)

Perhaps we could use a stored reference to it?

let bind = Function.prototype.bind;

function createFactory(type) {
    return bind.call(createElement, null, type);
}

[laysent]

Hi @developit Thanks a lot for the feedback. I am afraid the stored reference version might not avoid the potential problem I mentioned before, since if other code runs earlier, the stored reference might be an infected one. I agree with you that if there is potential performance issue after dropping .bind(), it might not worth doing it. I have made another commit which use .apply() to replace .bind() instead. .apply is a native API since the beginning while .bind is introduced since ECMAScript 5. There might be old libraries implement their own .bind while .apply should remain safe.

[developit]

The apply solution is nice and provides good safety. Any idea what the net increase/decrease in size is in switching from bind?

[laysent]

@developit Following is what I got when running npm run size before and after switching from bind:

	using bind	using apply
size	3.7 kB	3.7 kB

No obvious decrease/increase in size

[developit]

I want to merge the Symbol.for check here, but I'm still not certain we want to swap the bind() out. Want to split that one off?

[laysent]

@developit Hi, sorry for the late reply. Yes, we could merge the Symbol.for check for now. I am not very sure how to split that one off, could you let me know the rough steps?