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

Unify context implementations #13140

Closed
wants to merge 4 commits into from
Closed

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jul 3, 2018

Based on #13139

Implements the legacy, string-based context API on top of the new context API. This doesn't save much in code size because most of the legacy stuff deals with merging and masking. Arguably, it's a conceptual complexity win.

I'm ambivalent about whether we land this.

Currently, context can only be read by a special type of component,
ContextConsumer. We want to add support to all fibers, including
classes and functional components.

Each fiber may read from one or more contexts. To enable quick, mono-
morphic access of this list, we'll store them on a fiber property.
unstable_read can be called anywhere within the render phase. That
includes the render method, getDerivedStateFromProps, constructors,
functional components, and context consumer render props.

If it's called outside the render phase, an error is thrown.
@acdlite acdlite changed the title Unify context APIs Unify context implementations Jul 3, 2018
@pull-bot
Copy link

pull-bot commented Jul 3, 2018

React: size: 🔺+2.4%, gzip: 🔺+1.7%

ReactDOM: size: 🔺+0.8%, gzip: 🔺+0.7%

Details of bundled changes.

Comparing: 6731bfb...eb84045

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +0.8% +0.8% 56.81 KB 57.26 KB 15.82 KB 15.95 KB UMD_DEV
react.production.min.js 🔺+2.4% 🔺+1.7% 6.86 KB 7.03 KB 2.91 KB 2.96 KB UMD_PROD
react.development.js +0.9% +0.9% 51 KB 51.46 KB 13.99 KB 14.12 KB NODE_DEV
react.production.min.js 🔺+2.8% 🔺+2.3% 5.86 KB 6.03 KB 2.52 KB 2.57 KB NODE_PROD
React-dev.js +0.9% +0.9% 48.58 KB 49.04 KB 13.26 KB 13.38 KB FB_WWW_DEV
React-prod.js 🔺+2.4% 🔺+1.9% 13.38 KB 13.71 KB 3.73 KB 3.8 KB FB_WWW_PROD

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% -0.0% 627.88 KB 628.51 KB 146.85 KB 146.79 KB UMD_DEV
react-dom.production.min.js 🔺+0.8% 🔺+0.7% 95.52 KB 96.28 KB 30.85 KB 31.06 KB UMD_PROD
react-dom.development.js +0.1% -0.0% 624 KB 624.64 KB 145.67 KB 145.62 KB NODE_DEV
react-dom.production.min.js 🔺+0.8% 🔺+0.5% 95.51 KB 96.26 KB 30.39 KB 30.53 KB NODE_PROD
ReactDOM-dev.js +0.2% -0.0% 630 KB 631.22 KB 143.94 KB 143.93 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+1.7% 🔺+0.8% 268.64 KB 273.09 KB 50.62 KB 51.01 KB FB_WWW_PROD
react-dom.profiling.min.js +0.8% +0.4% 96.48 KB 97.23 KB 30.73 KB 30.86 KB NODE_PROFILING
ReactDOM-profiling.js +1.6% +0.8% 271.26 KB 275.71 KB 51.2 KB 51.61 KB FB_WWW_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% 411.86 KB 412.5 KB 92.41 KB 92.3 KB UMD_DEV
react-art.production.min.js 🔺+0.9% 🔺+0.9% 82.66 KB 83.41 KB 25.45 KB 25.67 KB UMD_PROD
react-art.development.js +0.2% -0.1% 343.84 KB 344.48 KB 75.27 KB 75.2 KB NODE_DEV
react-art.production.min.js 🔺+1.6% 🔺+1.1% 47.64 KB 48.39 KB 14.83 KB 14.99 KB NODE_PROD
ReactART-dev.js +0.4% 0.0% 332.64 KB 333.86 KB 70.01 KB 70.01 KB FB_WWW_DEV
ReactART-prod.js 🔺+2.9% 🔺+1.4% 139.51 KB 143.58 KB 23.92 KB 24.27 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% 348.35 KB 348.99 KB 75.83 KB 75.75 KB UMD_DEV
react-test-renderer.production.min.js 🔺+1.5% 🔺+1.2% 48.53 KB 49.28 KB 14.89 KB 15.08 KB UMD_PROD
react-test-renderer.development.js +0.2% -0.1% 344.47 KB 345.11 KB 74.85 KB 74.77 KB NODE_DEV
react-test-renderer.production.min.js 🔺+1.5% 🔺+1.4% 48.24 KB 48.99 KB 14.71 KB 14.91 KB NODE_PROD
ReactTestRenderer-dev.js +0.3% 0.0% 347.69 KB 348.91 KB 73.58 KB 73.59 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% 333.16 KB 333.8 KB 71.39 KB 71.32 KB NODE_DEV
react-reconciler.production.min.js 🔺+1.6% 🔺+1.3% 47.16 KB 47.89 KB 14.13 KB 14.32 KB NODE_PROD
react-reconciler-persistent.development.js +0.2% -0.1% 331.78 KB 332.41 KB 70.83 KB 70.74 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+1.6% 🔺+1.3% 47.17 KB 47.91 KB 14.14 KB 14.33 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.3% 0.0% 466.84 KB 468.1 KB 102.79 KB 102.82 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+1.9% 🔺+1.0% 203.5 KB 207.44 KB 35.74 KB 36.08 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.3% 0.0% 466.57 KB 467.84 KB 102.73 KB 102.76 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+1.9% 🔺+1.0% 199.61 KB 203.31 KB 34.98 KB 35.32 KB RN_OSS_PROD
ReactFabric-dev.js +0.3% 0.0% 457.12 KB 458.38 KB 100.38 KB 100.42 KB RN_FB_DEV
ReactFabric-prod.js 🔺+1.9% 🔺+1.1% 191.87 KB 195.57 KB 33.57 KB 33.92 KB RN_FB_PROD
ReactFabric-dev.js +0.3% 0.0% 457.15 KB 458.42 KB 100.4 KB 100.44 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+1.9% 🔺+1.1% 191.9 KB 195.6 KB 33.58 KB 33.94 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +1.8% +1.0% 202.29 KB 205.98 KB 35.58 KB 35.92 KB RN_OSS_PROFILING
ReactFabric-profiling.js +1.9% +1.1% 194.26 KB 197.95 KB 34.1 KB 34.47 KB RN_OSS_PROFILING
ReactNativeRenderer-profiling.js +1.9% +0.9% 206.13 KB 210.08 KB 36.34 KB 36.69 KB RN_FB_PROFILING
ReactFabric-profiling.js +1.9% +1.1% 194.22 KB 197.91 KB 34.08 KB 34.45 KB RN_FB_PROFILING

Generated by 🚫 dangerJS

@acdlite acdlite force-pushed the unifycontext-2 branch 4 times, most recently from fe64cbd to fe9e602 Compare July 3, 2018 06:51
Implements the legacy, string-based context API on top of the new
context API. This doesn't save much in code size because most of the
legacy stuff deals with merging and masking. Arguably, it's a conceptual
complexity win. I'm ambivalent about whether we land this.
Doing this in a separate commit so the diff is less confusing
@sophiebits
Copy link
Collaborator

Would this break unstable_renderSubtreeIntoContainer (#12493)?

@philipp-spiess
Copy link
Contributor

@sophiebits 🤔I was commenting on the wrong issue in the morning. I had the very same question here: #13139 (comment) (just want to link it for completeness)

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.

I don't think we'll want to do this if it is a breaking change. More likely, we'd just deprecate the legacy context.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 4, 2018

To clarify, it’s not a breaking change. In my comment above I meant that we don’t plan to make unstable_renderSubtreeIntoContainer work with the createContext API. It still works with the legacy API in this PR, using the same flawed approach as in the current implementation.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 4, 2018

But I’d also prefer to just deprecate legacy context :D

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 16, 2018

Not worth it

@acdlite acdlite closed this Jul 16, 2018
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.

None yet

6 participants