Skip to content
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

Fix lazy() with defaultProps #14112

Merged
merged 14 commits into from
Nov 6, 2018
Merged

Fix lazy() with defaultProps #14112

merged 14 commits into from
Nov 6, 2018

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 5, 2018

Builds on failing tests from #14108.
Fixes #13960.

I might have missed something. Just pushing it before going home. I haven't thought about the perf impact of this. Maybe we need to change the strategy and store them?

@gaearon gaearon changed the title Lazy fix Fix lazy() with defaultProps Nov 5, 2018
@gaearon
Copy link
Collaborator Author

gaearon commented Nov 5, 2018

Btw this probably means DevTools would also be wrong since they read off memoizedProps. And Enzyme & co.

@sizebot
Copy link

sizebot commented Nov 5, 2018

ReactDOM: size: -4.6%, gzip: -5.1%

Details of bundled changes.

Comparing: fd1256a...40aeb2e

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js -1.0% -0.5% 96.35 KB 95.39 KB 25.22 KB 25.09 KB UMD_DEV
react.development.js -1.6% -0.9% 60.48 KB 59.52 KB 16.26 KB 16.12 KB NODE_DEV
React-dev.js -0.0% -0.0% 56.97 KB 56.97 KB 15.34 KB 15.33 KB FB_WWW_DEV
React-prod.js 🔺+9.4% 🔺+6.0% 13.7 KB 14.99 KB 3.81 KB 4.04 KB FB_WWW_PROD
React-profiling.js +9.4% +6.0% 13.7 KB 14.99 KB 3.81 KB 4.04 KB FB_WWW_PROFILING

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 704.94 KB 705.8 KB 163.39 KB 163.48 KB UMD_DEV
react-dom.production.min.js -4.6% -5.1% 102.68 KB 98 KB 33.58 KB 31.86 KB UMD_PROD
react-dom.development.js +0.1% +0.1% 700.25 KB 701.11 KB 162.01 KB 162.11 KB NODE_DEV
react-dom.production.min.js -4.6% -5.1% 102.77 KB 98 KB 33.12 KB 31.42 KB NODE_PROD
react-dom-test-utils.development.js -2.1% -1.2% 45.83 KB 44.87 KB 12.44 KB 12.29 KB UMD_DEV
react-dom-test-utils.development.js -2.1% -1.2% 45.55 KB 44.59 KB 12.38 KB 12.23 KB NODE_DEV
react-dom-unstable-native-dependencies.development.js -1.6% -0.9% 61.56 KB 60.6 KB 16.05 KB 15.91 KB UMD_DEV
react-dom-unstable-native-dependencies.development.js -1.6% -0.9% 61.24 KB 60.28 KB 15.92 KB 15.78 KB NODE_DEV
react-dom-server.browser.development.js -0.6% -0.3% 118.4 KB 117.7 KB 31.35 KB 31.26 KB UMD_DEV
react-dom-server.browser.production.min.js -8.9% -9.2% 17.58 KB 16.02 KB 6.71 KB 6.09 KB UMD_PROD
react-dom-server.browser.development.js -0.6% -0.3% 114.53 KB 113.83 KB 30.39 KB 30.31 KB NODE_DEV
react-dom-server.browser.production.min.js -8.9% -9.2% 17.48 KB 15.92 KB 6.7 KB 6.09 KB NODE_PROD
react-dom-server.node.development.js -0.6% -0.3% 116.45 KB 115.75 KB 30.93 KB 30.84 KB NODE_DEV
react-dom-server.node.production.min.js -8.5% -8.7% 18.28 KB 16.72 KB 7 KB 6.39 KB NODE_PROD
ReactDOM-dev.js +0.3% +0.2% 718.73 KB 721.09 KB 162.87 KB 163.22 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.1% -0.3% 310.78 KB 311.17 KB 57.4 KB 57.21 KB FB_WWW_PROD
ReactDOMServer-dev.js +0.2% +0.2% 114.63 KB 114.89 KB 29.87 KB 29.92 KB FB_WWW_DEV
ReactDOMServer-prod.js 🔺+0.2% 🔺+0.1% 40.58 KB 40.65 KB 9.57 KB 9.58 KB FB_WWW_PROD
react-dom.profiling.min.js -4.2% -4.4% 105.13 KB 100.77 KB 33.43 KB 31.95 KB NODE_PROFILING
ReactDOM-profiling.js +0.7% +0.3% 315.06 KB 317.38 KB 58.29 KB 58.46 KB FB_WWW_PROFILING
react-dom.profiling.min.js -4.1% -4.2% 105 KB 100.68 KB 33.93 KB 32.5 KB UMD_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.2% +0.1% 491.55 KB 492.42 KB 108.95 KB 109.05 KB UMD_DEV
react-art.production.min.js -4.6% -5.1% 94.46 KB 90.12 KB 29.16 KB 27.66 KB UMD_PROD
react-art.development.js +0.2% +0.1% 423.34 KB 424.2 KB 91.92 KB 92.01 KB NODE_DEV
react-art.production.min.js -7.4% -8.1% 59.5 KB 55.1 KB 18.49 KB 16.98 KB NODE_PROD
ReactART-dev.js +0.5% +0.4% 428.16 KB 430.51 KB 90.48 KB 90.82 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.6% -0.0% 183.64 KB 184.66 KB 31.51 KB 31.51 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.2% +0.1% 436 KB 436.86 KB 94.59 KB 94.7 KB UMD_DEV
react-test-renderer.production.min.js -7.2% -8.4% 60.7 KB 56.32 KB 18.87 KB 17.28 KB UMD_PROD
react-test-renderer.development.js +0.2% +0.1% 431.22 KB 432.08 KB 93.45 KB 93.55 KB NODE_DEV
react-test-renderer.production.min.js -7.3% -8.2% 60.38 KB 55.99 KB 18.64 KB 17.12 KB NODE_PROD
react-test-renderer-shallow.development.js -3.6% -1.9% 26.74 KB 25.78 KB 7.17 KB 7.03 KB UMD_DEV
react-test-renderer-shallow.development.js -4.5% -2.3% 21.13 KB 20.17 KB 5.73 KB 5.6 KB NODE_DEV
ReactTestRenderer-dev.js +0.5% +0.4% 436.23 KB 438.58 KB 92.34 KB 92.67 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.2% +0.1% 421.18 KB 422.04 KB 90.35 KB 90.47 KB NODE_DEV
react-reconciler.production.min.js -7.4% -7.9% 60.72 KB 56.24 KB 18.28 KB 16.83 KB NODE_PROD
react-reconciler-persistent.development.js +0.2% +0.1% 419.63 KB 420.49 KB 89.73 KB 89.84 KB NODE_DEV
react-reconciler-persistent.production.min.js -7.4% -7.9% 60.73 KB 56.25 KB 18.29 KB 16.84 KB NODE_PROD
react-reconciler-reflection.development.js -5.9% -2.7% 16.35 KB 15.39 KB 4.96 KB 4.83 KB NODE_DEV

create-subscription

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
create-subscription.development.js -10.3% -4.8% 9.29 KB 8.33 KB 3.03 KB 2.89 KB NODE_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.3% +0.2% 554.44 KB 556.24 KB 121.35 KB 121.61 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.7% 🔺+0.5% 238.53 KB 240.21 KB 42.02 KB 42.22 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.3% +0.2% 554.11 KB 555.94 KB 121.25 KB 121.52 KB RN_OSS_DEV
ReactNativeRenderer-prod.js -5.4% -6.9% 238.55 KB 225.65 KB 42.02 KB 39.14 KB RN_OSS_PROD
ReactFabric-dev.js +0.4% +0.2% 544.63 KB 546.56 KB 118.86 KB 119.13 KB RN_FB_DEV
ReactFabric-prod.js -5.8% -7.1% 233.42 KB 219.9 KB 40.72 KB 37.82 KB RN_FB_PROD
ReactFabric-dev.js +0.4% +0.2% 544.66 KB 546.59 KB 118.88 KB 119.14 KB RN_OSS_DEV
ReactFabric-prod.js -5.8% -7.1% 233.45 KB 219.93 KB 40.74 KB 37.84 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js -5.5% -6.7% 244.24 KB 230.84 KB 43.23 KB 40.33 KB RN_OSS_PROFILING
ReactFabric-profiling.js -5.8% -7.0% 237.99 KB 224.2 KB 41.99 KB 39.07 KB RN_OSS_PROFILING
ReactNativeRenderer-profiling.js +0.6% +0.4% 244.22 KB 245.65 KB 43.23 KB 43.41 KB RN_FB_PROFILING
ReactFabric-profiling.js -5.8% -7.0% 237.95 KB 224.16 KB 41.97 KB 39.05 KB RN_FB_PROFILING

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

react-cache

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-cache.development.js -9.8% -4.2% 9.76 KB 8.8 KB 3.08 KB 2.95 KB NODE_DEV
react-cache.development.js -9.6% -4.0% 9.99 KB 9.03 KB 3.15 KB 3.02 KB UMD_DEV

Generated by 🚫 dangerJS

@@ -12,6 +12,21 @@ import type {LazyComponent, Thenable} from 'shared/ReactLazyComponent';
import {Resolved, Rejected, Pending} from 'shared/ReactLazyComponent';
import warning from 'shared/warning';

export function resolveDefaultProps(Component, baseProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not just used for lazy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it was this would be the wrong place for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can move to shared and reuse from ReactElement too. Couldn’t find a nice existing place.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t like shared. I prefer copying to putting things in shared. Makes it impossible to special case and encourages abstracting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So where do you want it? I guess I can create a file in the reconciler package but I don’t see why it deserves that when conceptually it’s related to our design for lazy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept it in LazyComponent for now because I don't want to duplicate it in reconciler bundle twice.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be called all the time. Make it conditional on type not equaling elementType.

@sebmarkbage
Copy link
Collaborator

It made sense to do this lazily when we only did this on render. But now we do the work multiple times. Not sure that makes sense.

@gaearon gaearon force-pushed the lazy-fix branch 2 times, most recently from 0083e4b to ed49616 Compare November 6, 2018 15:48
@gaearon
Copy link
Collaborator Author

gaearon commented Nov 6, 2018

This should not be called all the time. Make it conditional on type not equaling elementType.

Pushed.

Brandon Dail and others added 14 commits November 6, 2018 16:56
This is an optimization. I'm not sure it's entirely safe. It's probably worth running internal tests and see if we can ever trigger a case where they're different.

This can mess with resuming.
This reverts part of the previous commit. It broke a test that verifies we use current props in componentWillUnmount if the fiber unmounts due to an error.
@gaearon gaearon merged commit f777d19 into facebook:master Nov 6, 2018
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
* Resolve defaultProps for Lazy components

* Make test fail again

* Undo the partial fix

* Make test output more compact

* Add a separate failing test for sync mode

* Clean up tests

* Add another update to both tests

* Resolve props for commit phase lifecycles

* Resolve prevProps for begin phase lifecycles

* Resolve prevProps for pre-commit lifecycles

* Only resolve props if element type differs

* Fix Flow

* Don't set instance.props/state during commit phase

This is an optimization. I'm not sure it's entirely safe. It's probably worth running internal tests and see if we can ever trigger a case where they're different.

This can mess with resuming.

* Keep setting instance.props/state before unmounting

This reverts part of the previous commit. It broke a test that verifies we use current props in componentWillUnmount if the fiber unmounts due to an error.
n8schloss pushed a commit to n8schloss/react that referenced this pull request Jan 31, 2019
* Resolve defaultProps for Lazy components

* Make test fail again

* Undo the partial fix

* Make test output more compact

* Add a separate failing test for sync mode

* Clean up tests

* Add another update to both tests

* Resolve props for commit phase lifecycles

* Resolve prevProps for begin phase lifecycles

* Resolve prevProps for pre-commit lifecycles

* Only resolve props if element type differs

* Fix Flow

* Don't set instance.props/state during commit phase

This is an optimization. I'm not sure it's entirely safe. It's probably worth running internal tests and see if we can ever trigger a case where they're different.

This can mess with resuming.

* Keep setting instance.props/state before unmounting

This reverts part of the previous commit. It broke a test that verifies we use current props in componentWillUnmount if the fiber unmounts due to an error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants