[core] forward innerRef for certain components #14536
Conversation
|
Could we run the CI? |
Sure. But it's far from ready yet and I didn't want to waste resources. |
@eps1lon Thanks, I was interested in the bundle size. My biggest issue is how we gonna educate people to correctly apply the ref when implementing custom components. 90% of the libraries of the community doesn't correctly forward the reference. I have just tested react-router. I can't get a ref on the DOM an element when using the |
|
You can patch This approach should work with most libraries. As far as I know most of the libraries have some sort of Come to think of it it might be possible to check if a component allows refs to the DOM node ( Overall even if this adds to much complexity we can still cherry-pick changes from here. Things like |
eps1lon
force-pushed
the
feat/forwar-ref
branch
from
87273f5
to
49247e9
Compare
eps1lon
force-pushed
the
feat/forwar-ref
branch
5 times, most recently
from
e426edc
to
786fcf5
Compare
|
@material-ui/core: parsed: +0.81% , gzip: +0.44% Details of bundle changes.Comparing: 56fcebb...827b466
|
eps1lon
force-pushed
the
feat/forwar-ref
branch
3 times, most recently
from
730364e
to
6491c4c
Compare
- forwardRef for TouchRipple was a bad idea. We need the imperative handles of the class
Caused all tests to fail. Defering until absolutely necessary
[skip ci]
- added accidentally deleted component JSDOCs - ignored innerRef from withForwardRef HOC
oliviertassinari
force-pushed
the
feat/forwar-ref
branch
from
6491c4c
to
eadc6b9
Compare
oliviertassinari
force-pushed
the
feat/forwar-ref
branch
from
3e4c4d1
to
f8e6881
Compare
oliviertassinari
force-pushed
the
feat/forwar-ref
branch
from
f8e6881
to
827b466
Compare
|
@eps1lon We have lost some test coverage:
I'm not sure yet what's going on. Codecov reports 100% test coverage, and yet, it's not the case after the merge. Can we trust codecov 🤔? |
Little bit different approach this time: Refactored tests first then implementation. Definitely still biased because I knew how the implementation would look like but still increased confidence in tests for me. I need to investigate the shallow + disableLifeCycleIssue. This is causing enzyme to call componentDidMount if we wrap it in `forwardRef`. Continues #14536
It's recommended to review one commit at a time.
Two reasons I already open this:
Breaking
Wraps components in
forwardRef. Since every component is wrapped inwithStylesthis will only affect theinnerRefprop. They will no longer return a ref to the instance (or error for function components) but rather the underlying DOM node where useful1.Some components are not included (yet). Those are the ones where virtually no test was passing so I defer them to a separate PR:
The initial change broke about 25% of our unit tests. Some of them were due to own mistakes, some because
forwardRefwas not appropriate but overall I suspect 15% of the breakage was due to flawed tests. I think https://blog.kentcdodds.com/why-i-never-use-shallow-rendering-c08851a68bb7 is a pretty good starting point to explain why.Implementation details
const Foo = React.forwardRef(function Foo() {})is quite tricky WRT to whatFooactually referenceswithForwardRefHOC that injects thereffromforwardRefintoinnerRef. Since that prop is intercepted bywithStylesit does not cause any property collision.Notes for reviewers
Looking through one function and one class component implementation is sufficient. The rest follow the same pattern. The critical parts are the test files.
1 This excludes
TouchRipple. We need imperative handles here (seeuseImperativeHandleto trigger pulsating).