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

feat: transparent class wrapping #1261

Merged
merged 2 commits into from
Jun 10, 2019
Merged

feat: transparent class wrapping #1261

merged 2 commits into from
Jun 10, 2019

Conversation

theKashey
Copy link
Collaborator

@theKashey theKashey commented May 31, 2019

WIP

  • moves class wrapper to a fiber level removing a parasite side effect from a user level.

This is a major milestone for v5 and removes one of the biggest RHL's sideeffect type comparison - #304

State before

hot-loader/react-dom introduces controller type comparison. We injected a few things to react-dom

  • hotCleanup, to cleanup hooks rendering
  • hotRenderWithHooks, to be able to render hooks in hotReplacementRender
  • setHotElementComparator, to control type comparison, injected into reconcileSingleElement

New

This PR adds one more injection

  • setHotTypeResolver injected into createFiberFromTypeAndProps

How it works

  • no more user-space proxying
  • Elements are passed to as-is, so <Type/>.type==Type both for FC(worked before), and now for Classes
  • On fiber creation we are replacing the original type but a proxy
  • hotElementComparator, which compares fiber node with element now compares proxy with the original type, and have to wrap element to perform comparison.

This is a replacement for #1138

Left to do

  • changing hook order still throws an error
  • classes are still could not be fully updated due to unrepeatable constructors.

I recon the second issue could be if not skipped, then deprecated due to the hooks.

@codecov-io
Copy link

Codecov Report

Merging #1261 into master will decrease coverage by 0.22%.
The diff coverage is 82.75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1261      +/-   ##
=========================================
- Coverage   80.83%   80.6%   -0.23%     
=========================================
  Files          37      37              
  Lines        1273    1279       +6     
  Branches      329     332       +3     
=========================================
+ Hits         1029    1031       +2     
- Misses        191     194       +3     
- Partials       53      54       +1
Impacted Files Coverage Δ
src/reconciler/componentComparator.js 9.37% <50%> (-0.15%) ⬇️
src/reactHotLoader.js 70.88% <85.18%> (-2.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d74c7d9...9fe4cad. Read the comment docs.

@codecov-io
Copy link

codecov-io commented May 31, 2019

Codecov Report

Merging #1261 into master will decrease coverage by 0.21%.
The diff coverage is 82.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1261      +/-   ##
==========================================
- Coverage   80.83%   80.62%   -0.22%     
==========================================
  Files          37       37              
  Lines        1273     1321      +48     
  Branches      329      347      +18     
==========================================
+ Hits         1029     1065      +36     
- Misses        191      201      +10     
- Partials       53       55       +2
Impacted Files Coverage Δ
src/reconciler/componentComparator.js 9.37% <50%> (-0.15%) ⬇️
src/reactHotLoader.js 70.51% <84.61%> (-2.47%) ⬇️
src/AppContainer.prod.js 50% <0%> (-50%) ⬇️
src/hot.prod.js 71.42% <0%> (-28.58%) ⬇️
src/babel.prod.js 91.54% <0%> (-0.76%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d74c7d9...33bfe26. Read the comment docs.

@theKashey theKashey added this to the v5 milestone Jun 1, 2019
@theKashey
Copy link
Collaborator Author

@gaearon - I need your eye on this. This mitigates the major RHL issue, but injects more and more code into react internals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants