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.

Sign up for GitHub

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

Updates DirectionProvider to use componentDidUpdate instead of UNSAFE_componentWillReceiveProps #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+22 −6
Open

Updates DirectionProvider to use componentDidUpdate instead of UNSAFE_componentWillReceiveProps #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/DirectionProvider.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ export default class DirectionProvider extends React.Component {
};
}

componentWillReceiveProps(nextProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The children of DirectionProvider need the direction to be able to render properly; so by setting this earlier, we potentially avoid multiple extra renders.

Copy link
Author

Choose a reason for hiding this comment

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

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.).

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

if (this.props.direction !== nextProps.direction) {
this.broadcast.setState(nextProps.direction);
componentDidUpdate(prevProps) {
if (prevProps.direction !== this.props.direction) {
this.broadcast.setState(this.props.direction);
}
}

Expand Down
15 changes: 12 additions & 3 deletions tests/DirectionProvider_test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
/**
* @jest-environment jsdom
*/

// We must set the env to jsdom for this file to accommodate mounted components.
// The components must be mounted because there is currently no way to test
// code within componentDidUpdate lifecycle in shallow mounting.
// https://github.com/airbnb/enzyme/issues/1452

import React from 'react';
import { expect } from 'chai';
import { shallow } from 'enzyme';
import { shallow, mount } from 'enzyme';
import sinon from 'sinon-sandbox';

import DirectionProvider from '../src/DirectionProvider';
Expand Down Expand Up @@ -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(
Copy link
Collaborator

Choose a reason for hiding this comment

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

please do not alter these tests; these must remain with shallow, and new tests added to cover these changes.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you link to that enzyme issue?

tbh I'd rather wait for that to be resolved.

Copy link
Author

Choose a reason for hiding this comment

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

Meant to include it in my notes above. Here it is (I'll add it above as well): enzymejs/enzyme#1452

<DirectionProvider direction={direction}>{children}</DirectionProvider>,
);
const broadcast = wrapper.instance().broadcast;
Expand All @@ -52,7 +61,7 @@ describe('<DirectionProvider>', () => {
it('does not broadcast the direction when the direction prop stays the same', () => {
const direction = DIRECTIONS.LTR;
const nextDirection = DIRECTIONS.LTR;
const wrapper = shallow(
const wrapper = mount(
<DirectionProvider direction={direction}>{children}</DirectionProvider>,
);
const broadcast = wrapper.instance().broadcast;
Expand Down
1 change: 1 addition & 0 deletions tests/_helpers.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import sinon from 'sinon-sandbox';
import chaiEnzyme from 'chai-enzyme';

import configure from 'enzyme-adapter-react-helper';
import './shim';

configure({ disableLifecycleMethods: true });

Expand Down
6 changes: 6 additions & 0 deletions tests/shim.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// This file is required to alleviate an RAF error that appears:
// https://github.com/facebook/jest/issues/4545

global.requestAnimationFrame = (callback) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not needed; airbnb-browser-shims should provide all the shims we need including this one.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hm, that seems like something we should fix - would you mind filing an issue on browser-shims for that?

Copy link
Author

Choose a reason for hiding this comment

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

Yeppers

Copy link
Author

Choose a reason for hiding this comment

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

airbnb/browser-shims#20

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is now resolved, by an update of matchmedia-polyfill.

setTimeout(callback, 0);
};