-
Notifications
You must be signed in to change notification settings - Fork 46.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove PooledClass from isomorphic build #10227
Conversation
scripts/rollup/results.json
Outdated
"size": 27099, | ||
"gzip": 7215 | ||
"size": 24229, | ||
"gzip": 6612 |
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.
whoa. why is FB_PROD so much larger than NODE_PROD/UMD_PROD? Legacy files that are being consumed internally yet?
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.
No, it's just not minified. We might minify it eventually but for now it doesn't seem necessary (it goes through our pipeline anyway), and it's easier to see what happens during the sync.
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.
Ah! Great point. 👍
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.
LGTM!
@@ -164,7 +353,7 @@ function mapChildren(children, func, context) { | |||
return result; | |||
} | |||
|
|||
function forEachSingleChildDummy(traverseContext, child, name) { |
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.
We're already importing the emptyFunction
library in ReactChildren
, could use just use emptyFunction.thatReturnsNull
and get rid of this?
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.
Addressed.
* @param {string} key to be escaped. | ||
* @return {string} the escaped key. | ||
*/ | ||
function escape(key: string): string { |
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.
Hey,
Why you added types in flow style if this file have't flow comment?
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.
Probably accidentally or by copy paste. Want to send a PR to Flow-ify it?
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.
yep! I can delete useless flow typechecking or add comment flow. If I add flow comment maybe I can try to update types for all file? If you want this.
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.
Sure.
This inlines
traverseAllChildren
logic intoReactChildren
since it’s now the only consumer. Then I changeforEach
andmap
to use the same bookkeeping fields. This lets me unroll thePooledClass
abstraction into a single specific implementation, and thus remove it from the isomorphic build.See individual commits.