Refactor React.Children to reduce indirection #18332
Conversation
All usages are internal so we can simply use the inner function directly.
This isn't useful by itself but makes the layering easier to follow. traverseAllChildren is only used at the lowest layer now.
Get rid of the traversal context. Use closures.
facebook-github-bot
added
the
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 98902bb:
|
Details of bundled changes.Comparing: 2666642...98902bb react
React: size: -3.6%, gzip: -3.1% Size changes (experimental) |
Details of bundled changes.Comparing: 2666642...98902bb react
React: size: -3.9%, gzip: -3.0% Size changes (stable) |
|
I'm pretty sure I'm right (c) I can write a small one-off generative test thing to verify the results match between implementations. #covid #bored |
|
I know it's subtle though. E.g. people likely depend even on the nested map unwrapping call order. I think I kept all of that but I agree it's worth testing more closely. |
|
wdyt of just dropping the pooling and closure avoidance but keeping traverseAllChildren with a different callback for map vs forEach vs count? I think that gets you 90% of the code simplification. |
|
and is nicer in some ways – eg: count doesn't need to make an array. |
|
I guess that would be better than what we have today, but I'd still prioritize golfing code size over runtime perf of these helpers. |
|
Worth noting traverse perf was super important when it was shared by the reconciler code. It's not anymore though. |
There was a problem hiding this comment.
The escape algorithm is interesting. Several extra objects like regexps and closures per entry. Even for things like forEach and count that doesn't need the keys.
I couldn't find a bug after a few spot checks. I didn't carefully review it all but it's at least an indication that there's probably at least not many bugs.
|
Let's give it a try. The only discrepancy I found is that |
This has been bugging me for a while. I know we don't plan to change their behavior because it's a legacy pattern anyway (although we don't have an alternative either). But it's a bit awkward to see this indirection in the bundle. It's also very confusing to read.
The original code was written assuming this is perf critical and we must avoid closures. I don't think it is. This pattern isn't super common and when you have it, you probably don't want to iterate over thousands of these anyway. In either case, my last implementation has minimal use of closures too. I removed pooling too which is a bit over-engineered for this use case and might actually hurt perf. Unlike with events, it's not observable.
I re-implemented count, forEach, and toArray in terms of map. I know they have slightly different semantics so a naïve implementation wouldn't work but I think I got it right (and when I did it wrong, tests failed). The original rationale to implement them separately was to reuse a lower level primitive that doesn't require any extra allocations. But since both
forEachandcounthave other quirks (like counting holes) I don't expect them to actually be used that much in critical paths anymore. So I think it's okaycountallocates an array now and later throws it away.Each commit is fairly mechanical. At the end, I have a single recursive function as a core of map, and other functions expressed through map. Instead of a tangled web of indirections. Read individual commits. Although I guess reading the whole thing also works because you can follow it now.