createChainedFunction improvements

[mtscout6]

Changes createChainedFunction to chain many functions, and to throw if non-functions are provided.

Reasons I'm proposing this to master and not v0.24.0-rc:

• This works against all our existing usage of the function
• Not throwing before sounds like a bug to me

@jtenner this introduces that improved function type check you proposed on gitter There is a test proving this wasn't necessary.
@taion it's going to throw now when something other than a func is provided, as you proposed on gitter.

[jquense]

bit of a nitpick, I am a bit confused about the toString call? what is it protecting against that typeof doesn't catch? it seems very unlikely that anyone is using new Function to create a callback. Only mentioning because it strikes me as harder to understand, and React itself doesn't make this check, so I am not sure there is any big need to support such an edge case.

[taion]

LGTM

[mtscout6]

@jtenner it was mentioned later in the day that I missed somehow till now by @AlexKVal that there are some performance concerns with the toString option. This lead me to write a unit test that uses the new Function() approach which works with the traditional typeof check.

[taion]

Since we're already pulling in lodash, maybe we should just use their isFunction.

[mtscout6]

Lodash is just a dev dependency right now.

On Sat, Jun 13, 2015, 10:59 Jimmy Jia notifications@github.com wrote:

Since we're already pulling in lodash, maybe we should just use their
isFunction.

—
Reply to this email directly or view it on GitHub
#821 (comment)
.

[taion]

My bad - misread the dependency listing.

I know lodash lets you import just the functions you need, e.g. lodash/lang/isFunction, and provides per-function packages (e.g. https://www.npmjs.com/package/lodash.isfunction). Might not be unreasonable to pull it in as a dependency?

[mtscout6]

Sounds reasonable to me! Looks like they are accounting for browser specific differences I haven't considered, even though there is a performance cost.

[jquense]

I still think its a bunch of extra code that isn't needed, I don't know how often someone is going to be confusing a callback with a U8IntArray (looking through the code) in practice, lodash can accidentally bring a lot of code in even with the modular builds, if we aren't careful. In this instance its like 1 line vs 10 so it might not matter that much.

[jquense]

ahh it looks like the default build in lodash 3 is the modern one, so it is probably trimmer than I thought

[mtscout6]

The issue that got me was when I was looking through the lodash sort I say that they were accounting for:

• avoid a Chakra JIT bug in compatibility modes of IE 11.
• The use of Object#toString avoids issues with the typeof operator in older versions of Chrome and Safari which return 'function' for regexes and Safari 8 equivalents which return 'object' for typed array constructors.

I'm not a huge browser version specific person but it seems as though they will handle those quirks better than we can over time.

[jquense]

I agree that it defiantely is better at cross compat then we probably are :) my thought was only that in this specific case the likelihood of any of those bugs biting us is very small, given that it is an undocumented util, that will almost always be fed functions pre validated by proptypes or class methods.

I am probably being too nitpicky here tho, I get a get a bit weird about byte counts :P

[taion]

We can check how much code from Lodash gets pulled in, but my thought here is just to make it easier to tell what's going on.

[taion]

FWIW I don't think there should be much of a benefit to using lodash.isfunction instead of just importing from lodash/lang/isFunction - should lead to all the bundles ending up the same size.

[jquense]

in fact the main lodash package will dedupe better for users or if we use more than just this function, b/c the common code will all be the same.

also the individual packages can create really long paths that us poor windows users can handle (though its mostly better in 3.0)

[mtscout6]

This is turning out to be a larger discussion, I propose to take it out for now and open an issue targeted at discussing it. Thoughts?

[AlexKVal]

I propose to do as React does:
They seem judged that typeof === 'function' is sufficient enough.

var isFunction = typeof property === 'function';

src/isomorphic/classic/class/ReactClass.js#L488

var ITERATOR_SYMBOL = typeof Symbol === 'function' && Symbol.iterator;

src/shared/utils/getIteratorFn.js#L16

typeof typeDef[propName] === 'function'

src/isomorphic/classic/class/ReactClass.js#L396

and so on many such in the React code base
🍒

[taion]

That's a really good point. I see that PropTypes does the same thing. We probably don't need to handle edge cases that React itself doesn't care about.

[mtscout6]

I'll toast to that! I'll update the PR.

[jtenner]

Alright. As a person who has used Object.prototype.toString in requestAnimationFrame loops, your performance concerns are valid.

I had no idea you guys had access to lodash at the time. PLEASE USE _.isFunction.

In fact, please use the following call to get your array of functions.
_.filter(arguments, _.isFunction)

Then use a reduce.
_.reduce(functions, executeFunction)

[mtscout6]

@jtenner The discussion about adding lodash as a dependency is partly what tripped this up. Currently lodash is just a dev dependency as a convenience in testing and build/release scripts. There are pros a cons to adding it as a direct dependency which I'm fine with discussing, but it should happen in it's own issue and not detract from this PR.

[AlexKVal]

Hmm.. windowsCI gives a lot of eslint warnings: error ... is defined but never used no-unused-vars
lets see what's going on..

[AlexKVal]

As for the code: LGTM
by (...funcs) we always got an array and can use .reduce(). Nice. ❇️

'herpa derpa' is kinda Matt's brand in tests 😄

[AlexKVal]

I've found the culprit. Creating the issue.
eslint-plugin-react@2.5.1 exactly this version.
We can safely downgrage and fix to 2.5.0 for the time.
It is not only windowsCI. It is for all npm run lint.

[AlexKVal]

Update: It is already addressed by jsx-eslint/eslint-plugin-react#110
It was in closed Issues. I wasn't looking into closed. Sorry for the wrong alert. 🍒

[AlexKVal]

Then I merge it.

[mtscout6]

By the way herpa derpa is actually the calling card of a friend of mine, I just adopted it cause I thought it was funny.

[AlexKVal]

nice 😄