Add support for custom factory methods #10
Conversation
aaronjensen
force-pushed
the
configurable-factory-methods
branch
2 times, most recently
from
b4afb12 to
dec29db
Compare
|
@gaearon rebased on master and fixed the test, lmk if there are any tweaks you'd like me to make. |
|
@gaearon hm any love on this? |
aaronjensen
force-pushed
the
configurable-factory-methods
branch
from
dec29db to
25e8ec0
Compare
|
Apologies for dragging my feet on this. I got sick and had a lot of catching up to do. |
|
No problem, hope you are feeling better! Thanks. Aaron On Sat, Sep 19, 2015 at 8:36 AM, Dan Abramov notifications@github.com
|
|
There is in fact something else which is tangential to this PR but I'd love to see it here. Currently we run all test cases with the same config, which is a problem for testing different configurations. For example, we can't test that the same code is treated differently with different configurations. What I'd like to see is Babel config being moved into separate Would you be willing to add this? |
| function isCreateClassCallExpression(node, factoryMethods) { | ||
| for (const method of factoryMethods) { | ||
| if (method.indexOf('.') !== -1) { | ||
| if (t.buildMatchMemberExpression(method)(node.callee)) { |
There was a problem hiding this comment.
This appears to have worse performance. (I haven't tested but the code looks so.) The point of buildMatchMemberExpression is to build the matching function and save it. However in this code we build the matching function on every invocation.
I think what we really need here is buildMatchCreateClassCallExpression(factoryMethods) which would return a node-accepting function. This way we should be able to create (and memoize) the matching function once per plugin invocation. Does this make sense?
There was a problem hiding this comment.
It's definitely faster to pre-build it, though I'm not sure how much the difference matters when just about everything is still in the single digit microsecond or faster range:
build inline x 2,421,358 ops/sec ±1.20% (91 runs sampled)
prebuilt x 11,400,386 ops/sec ±1.20% (92 runs sampled)
Fastest is prebuilt
build inline x 2,678,951 ops/sec ±1.01% (95 runs sampled)
prebuilt x 11,239,894 ops/sec ±1.51% (90 runs sampled)
Fastest is prebuilt
build inline x 2,634,932 ops/sec ±1.24% (94 runs sampled)
prebuilt x 10,768,092 ops/sec ±1.17% (94 runs sampled)
Fastest is prebuilt
build inline x 296,499 ops/sec ±1.74% (88 runs sampled)
prebuilt x 2,769,726 ops/sec ±0.94% (92 runs sampled)
Fastest is prebuilt
build inline x 1,382,369 ops/sec ±0.97% (93 runs sampled)
prebuilt x 2,686,110 ops/sec ±1.29% (93 runs sampled)
Fastest is prebuilt
build inline x 2,558,054 ops/sec ±1.10% (88 runs sampled)
prebuilt x 11,474,870 ops/sec ±1.19% (93 runs sampled)
Fastest is prebuilt
build inline x 2,553,266 ops/sec ±1.39% (95 runs sampled)
prebuilt x 10,736,413 ops/sec ±1.36% (91 runs sampled)
Fastest is prebuilt
build inline x 1,379,023 ops/sec ±1.25% (93 runs sampled)
prebuilt x 2,820,422 ops/sec ±1.03% (94 runs sampled)
Fastest is prebuilt
I'll tweak it to prebuild though, may as well.
Sure! ad8fb28 |
|
@gaearon All feedback addressed, let me know if there's anything else. |
| const factoryMethods = options.factoryMethods || ['React.createClass', 'createClass']; | ||
| this.state[optionsKey] = options; | ||
| this.state[cacheKey] = { | ||
| isCreateClassCallExpression: buildIsCreateClassCallExpression(factoryMethods), |
There was a problem hiding this comment.
I opted to build the matchers up front instead of memoizing them off of the method name. It seemed a little simpler/more efficient in the common case.
|
This is good, thank you. Will merge later today. |
|
@gaearon just merged master in to get back up to date, it was getting too painful to rebase all of the commits w/ all the conflicting readme changes. This still good to merge? |
|
Let's add a backward compatibility mode. I feel like changing all examples in boilerplates and READMEs etc is going to be a world of royal pain. I'd say let's allow array as a configuration, but print something like |
|
Do we still want to rename |
|
Meh, let's keep |
|
Ok, added backwards compat mode, including the transform/target rename (got your msg too late). Let me know what you think or if you'd like me to remove the rename. |
|
What do you think is better? I think |
|
Yeah, I didn't really get target either. I'd consider target to mean the files that it'd transform, not the transform it'd use. I'm good w/ the rename. IMO release a new major, remove backwards compat and release another major (maybe not all today 😄) |
|
I think the right way is to put support for this in a minor, wait for people to migrate, then update the docs everywhere, and then release a major. |
|
yeah that sounds like a better idea. Works for me. |
Add support for custom factory methods
|
This is out in 1.1.0, thank you very much. |
Fixes #8