Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

Fix potential compatibility issue with other libraries #440

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

laysent
Copy link

@laysent laysent commented Oct 21, 2017

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 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.

  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.

@laysent laysent changed the title improve fix of #423 Fix potential compatibility issue with other libraries Oct 26, 2017
@developit
Copy link
Member

developit commented Jan 30, 2018

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
Copy link
Author

laysent commented Jan 30, 2018

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.

Before ECMAScript 5, libraries define .bind without additional parameter. To work with these libraries, change this specific usage of bind
@developit
Copy link
Member

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

@developit developit added this to the 3.19 milestone Feb 23, 2018
@developit developit added this to To Do in Backlog via automation Feb 23, 2018
@developit developit moved this from To Do to In progress in Backlog Feb 23, 2018
@laysent
Copy link
Author

laysent commented Feb 27, 2018

@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

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);
Copy link
Member

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.

Copy link
Author

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.

@developit
Copy link
Member

developit commented May 25, 2018

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
Copy link
Author

laysent commented May 29, 2018

@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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Backlog
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants