Updates DirectionProvider to use componentDidUpdate instead of UNSAFE_componentWillReceiveProps #14
Conversation
dclowd9901
changed the title
| @@ -40,7 +49,7 @@ describe('<DirectionProvider>', () => { | |||
| it('broadcasts the direction when the direction prop changes', () => { | |||
| const direction = DIRECTIONS.LTR; | |||
| const nextDirection = DIRECTIONS.RTL; | |||
| const wrapper = shallow( | |||
| const wrapper = mount( | |||
There was a problem hiding this comment.
please do not alter these tests; these must remain with shallow, and new tests added to cover these changes.
There was a problem hiding this comment.
There is still an open issue on Enzyme being incapable of causing cDU to fire when setProps is called on the wrapper. The only known workaround is using mount over shallow. This change is isolated to this file, and only tests that need to ensure this behavior is working correctly.
There was a problem hiding this comment.
Can you link to that enzyme issue?
tbh I'd rather wait for that to be resolved.
There was a problem hiding this comment.
Meant to include it in my notes above. Here it is (I'll add it above as well): enzymejs/enzyme#1452
| // This file is required to alleviate an RAF error that appears: | ||
| // https://github.com/facebook/jest/issues/4545 | ||
|
|
||
| global.requestAnimationFrame = (callback) => { |
There was a problem hiding this comment.
this is not needed; airbnb-browser-shims should provide all the shims we need including this one.
There was a problem hiding this comment.
Unfortunately, blanket importing airbnb-browser-shims causes a nebulous parentNode is undefined error from matchmedia-polyfill. Alternatively we could piecemeal import the raf polyfill on its own.
There was a problem hiding this comment.
hm, that seems like something we should fix - would you mind filing an issue on browser-shims for that?
There was a problem hiding this comment.
This is now resolved, by an update of matchmedia-polyfill.
| @@ -38,9 +38,9 @@ export default class DirectionProvider extends React.Component { | |||
| }; | |||
| } | |||
|
|
|||
| componentWillReceiveProps(nextProps) { | |||
There was a problem hiding this comment.
This is still a safe method to use, despite React's deprecations; componentDidUpdate ends up happening later than cWRP. I'm not clear on why this change is helpful or desirable.
There was a problem hiding this comment.
Disregarding that this will break in the future, many warning messages are noisy and unhelpful when debugging or working with code. cWRP does happen before cDU but there's no clear reason the events in this function need to happen before a render update anyway, and the methods used in this PR align with React's recommendations on the matter.
There was a problem hiding this comment.
The children of DirectionProvider need the direction to be able to render properly; so by setting this earlier, we potentially avoid multiple extra renders.
There was a problem hiding this comment.
This is tricky. They've seemed to strip all *will* lifecycle hooks from "safeness". We could bypass the lifecycle completely and just export the broadcaster explicitly from DirectionProvider to withDirection and let the withDirection components handle their own updating, then we don't have to worry about repeat lifecycle. This actually seems more reasonable than arbitrarily using context to handle the broadcaster (realistically, DirectionProvider should only be used once per app, anyway, so there doesn't seem to be a need to for context, unless I'm misunderstanding.).
There was a problem hiding this comment.
I have a feeling that the proper solution here is feature-detecting the new React context API, and using that when available - otherwise falling back to the existing implementation, as-is, in master.
ljharb
changed the title
This change migrates DirectionProvider to cease using
UNSAFE_componentWillMountin favor ofcomponentDidUpdatein accordance withUnfortunately, since Enzyme lacks support for shallow mounting that fires
componentDidUpdate, there is also a tangential change that includesjsdomas the environment that DirectionProvider's test is run in. And further, because this causes a warning to appear in the test regardingrequestAnimationFrame, I used a method found in this thread to quell the warning: jestjs/jest#4545One last thing to note: Some of the testing here was switched to
mountin favor ofshallow. This was to accommodate an ongoing issue with Enzyme being unable to trigger a call to cDU withsetPropsbeing called on a shallow wrapper. enzymejs/enzyme#1452