-
-
Notifications
You must be signed in to change notification settings - Fork 148
Fix potential compatibility issue with other libraries #440
base: master
Are you sure you want to change the base?
Conversation
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 Perhaps we could use a stored reference to it? let bind = Function.prototype.bind;
function createFactory(type) {
return bind.call(createElement, null, type);
} |
Hi @developit Thanks a lot for the feedback. |
Before ECMAScript 5, libraries define .bind without additional parameter. To work with these libraries, change this specific usage of bind
(introduced by preactjs#289)
The apply solution is nice and provides good safety. Any idea what the net increase/decrease in size is in switching from bind? |
@developit Following is what I got when running
No obvious decrease/increase in size |
src/index.js
Outdated
@@ -218,7 +218,9 @@ let currentComponent; | |||
|
|||
|
|||
function createFactory(type) { | |||
return createElement.bind(null, type); | |||
return function () { | |||
return createElement.apply(type, arguments); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why the tests aren't failing, but I think this is missing an argument?
createElement.apply('div', 'a', { href: '#foo' })
That would seem to be setting this
to "div"
in createElement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I guess this implementation is wrong.
I could think of some ugly but correct implementation such as createElement.apply(null, [type].concat(Array.apply(null, arguments)))
.
In general, that's probably a bad idea and should not be merged. I will send another pr if I come up with some better idea.
I want to merge the |
@developit Hi, sorry for the late reply. Yes, we could merge the |
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)
First commit is to improve Symbol implementation check.
I saw IE11 seems to define Symbol but without the method 'for' #423 has been merged already. Hopefully it can be released soon.
This is only a tiny improvment of IE11 seems to define Symbol but without the method 'for' #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.
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: mootoolsSo far from what I know, it should be fine to use
.bind
to bindthis
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.