Conditionally use `PureComponent` instead of `Component` #1335

AntiFish03 · 2018-08-30T17:18:35Z

Working to resolve #1297. Conditionally using React.PureComponent instead of React.Component

…omponent` instead of `React.Component`

coveralls · 2018-08-30T17:27:35Z

Coverage increased (+0.2%) to 84.875% when pulling 88fc16a on getaroom:PureComponent-Upgrade into ebf83a1 on airbnb:master.

ljharb · 2018-08-30T17:48:44Z

src/components/CalendarDay.jsx

-class CalendarDay extends React.Component {
+const BaseClass = baseClass();
+
+class CalendarDay extends BaseClass {


this is going to break react linting; we'll need a jsdoc comment to indicate that it extends React.Component

ljharb · 2018-08-30T17:49:14Z

src/components/CalendarDay.jsx

@@ -48,17 +48,15 @@ const defaultProps = {
  phrases: CalendarDayPhrases,
 };

-class CalendarDay extends React.Component {
+const BaseClass = baseClass();


extends baseClass() works too

ljharb · 2018-08-30T17:50:14Z

src/utils/baseClass.js

+
+export const pureComponentAvailable = typeof React.PureComponent !== 'undefined';
+
+export default function baseClass(pureComponent = pureComponentAvailable) {


why is baseClass a function, but pureComponentAvailable is not?

I'd expect either both to be a function (to support polyfilling of PureComponent, i assume), or neither.

- Both `pureComponentAvailable` and `baseClass` are now functions - Using `extends baseClass()` instead of defining `BaseClass = baseClass()` - Fixing the lint on `DayPickerRangeController` for `pureComponentAvailable` being defined but not used

AntiFish03 · 2018-08-30T18:11:15Z

@ljharb what does the jsdoc comment need to be? Your linting system is different than I am used to working with.

AntiFish03 · 2018-08-30T18:13:46Z

Also, @majapw this PR only resolves most of the avoidable re-rendering issue. As I mentioned in #1297, I was not able to resolve some re-rendering on DayPickerNavigation or on DayPicker

ljharb · 2018-08-30T18:34:00Z

(what are you used to working with that’s not eslint?) i believe it’s /** @extends React.Component */ but I’ll have to check the docs and get back to you. You can test by introducing a lint error, verifying that it fails when it extends React.Component, verify that it passes when extending baseClass, and then verify that it fails with the JSdoc comment.

AntiFish03 · 2018-08-30T18:54:39Z

@ljharb not in that way. I guess I never knew about being able to do that is a better way of describing it.

ljharb · 2018-08-30T18:56:59Z

@AntiFish03 here's an example: jsx-eslint/eslint-plugin-react#513 (comment)

…nters.

ljharb · 2018-08-30T19:07:23Z

@AntiFish03 what's the reasoning that the baseClass stuff needs to be in a function, instead of caching it once at module load time?

AntiFish03 · 2018-08-30T20:02:46Z

It doesn't have to be. Just trying to keep it similar to what is used in react-with-styles as was requested.

…oad, instead of using a function.

AntiFish03 · 2018-08-30T21:49:03Z

travis-ci decided to have some timeout issues there for no apparent reason... @ljharb care to restart that and everything should be good to go

ljharb · 2018-08-30T22:42:02Z

After a few discussions, i wonder if it'd be worth it to try something like this?

/** @extends React.Component */
class Foo extends React.PureComponent || React.Component {
  [React.PureComponent || 'shouldComponentUpdate'](nextProps, nextState) {
     return shallowCompare(this, nextProps, nextState);		
  }
}

Otherwise, your change LGTM.

AntiFish03 · 2018-08-31T20:58:32Z

@ljharb, Change made as requested anything else?

ljharb

With appropriate documentation - including telling people that they can overwrite build/utils/baseClass.js with module.exports = React.PureComponent if they know they're using a newer React - this LGTM.

AntiFish03 · 2018-09-04T16:19:34Z

@ljharb, Readme updated.

majapw

This looks great! Thank you so much for making this change.

majapw · 2018-09-04T16:44:33Z

.storybook/config.js

@@ -1,4 +1,9 @@
 import React from 'react';
+if (process.env.NODE_ENV !== 'production') {
+  const {whyDidYouUpdate} = require('why-did-you-update');


nit: spaces around the whyDidYouUpdate

const { whyDidYouUpdate } = require('why-did-you-update');

majapw · 2018-09-04T16:50:39Z

src/utils/baseClass.js

+import shallowCompare from 'react-addons-shallow-compare';
+
+class BaseClass extends (React.PureComponent || React.Component) {
+  [React.PureComponent || 'shouldComponentUpdate'](nextProps, nextState) {


I'm not sure I fully understand what this will do in the case that React.PureComponent is defined

oh right, this should be React.PureComponent && - so that it's undefined in that case, instead of the method name

shouldn't it actually be !React.PureComponent && so that it injects the shouldComponentUpdate when PureComponent isn't available.

aha, yes, that's better. coding is hard.

We're all tired. :P

majapw

Gonna put an X on this temporarily so that || doesn't accidentally get merged in

AntiFish03 · 2018-09-04T22:41:44Z

@majapw, @ljharb There you go both of those pieces should be fixed now

majapw

majapw · 2018-09-12T00:50:36Z

So fun fact, I believe that this breaks react-dates in IE10. sigh It seems to be related to how this is transformed by babel (current working theory is something about the double-extends).
I'm seeing Cannot call a class as a function coming from https://unpkg.com/react-dates@18.0.2/lib/utils/baseClass.js.

This reverts commit f2536b6, reversing changes made to ebf83a1.

…Upgrade"" This reverts commit 50c382f.

AntiFish03 · 2018-09-24T21:28:44Z

@majapw all of a sudden I am getting

Warning: DateRangePickerInputController has a method called shouldComponentUpdate(). shouldComponentUpdate should not be used when extending React.PureComponent. Please extend React.Component if shouldComponentUpdate is used.

Is this something you are seeing also?

majapw · 2018-09-24T22:46:52Z

@AntiFish03 locally or in storybook? on the latest version?

AntiFish03 · 2018-09-25T20:55:06Z

@majapw, Locally from master.

Working to resolve react-dates#1297. Conditionally using `React.PureC…

e362c8e

…omponent` instead of `React.Component`

ljharb reviewed Aug 30, 2018

View reviewed changes

Making requested changes...

1935ac0

- Both `pureComponentAvailable` and `baseClass` are now functions - Using `extends baseClass()` instead of defining `BaseClass = baseClass()` - Fixing the lint on `DayPickerRangeController` for `pureComponentAvailable` being defined but not used

Adding JSDOC to allow eslint to properly apply the react component li…

65cf032

…nters.

Now both BaseClass and pureComponentAvailable are set at module l…

a2fc9ac

…oad, instead of using a function.

Making requested change

e230c61

ljharb reviewed Aug 31, 2018

View reviewed changes

ljharb requested review from majapw and lencioni August 31, 2018 23:20

Updating the readme about overriding base class

22c33ab

majapw approved these changes Sep 4, 2018

View reviewed changes

majapw requested changes Sep 4, 2018

View reviewed changes

couple of small tweaks.

88fc16a

ljharb approved these changes Sep 4, 2018

View reviewed changes

majapw approved these changes Sep 4, 2018

View reviewed changes

majapw merged commit f2536b6 into react-dates:master Sep 4, 2018

majapw added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Sep 5, 2018

majapw added a commit that referenced this pull request Sep 12, 2018

Revert "Merge pull request #1335 from getaroom/PureComponent-Upgrade"

50c382f

This reverts commit f2536b6, reversing changes made to ebf83a1.

majapw added a commit that referenced this pull request Sep 13, 2018

Revert "Revert "Merge pull request #1335 from getaroom/PureComponent-…

4f8eb01

…Upgrade"" This reverts commit 50c382f.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditionally use `PureComponent` instead of `Component` #1335

Conditionally use `PureComponent` instead of `Component` #1335

AntiFish03 commented Aug 30, 2018

coveralls commented Aug 30, 2018 •

edited

Loading

ljharb Aug 30, 2018

ljharb Aug 30, 2018

ljharb Aug 30, 2018

AntiFish03 commented Aug 30, 2018

AntiFish03 commented Aug 30, 2018

ljharb commented Aug 30, 2018

AntiFish03 commented Aug 30, 2018

ljharb commented Aug 30, 2018

ljharb commented Aug 30, 2018

AntiFish03 commented Aug 30, 2018

AntiFish03 commented Aug 30, 2018

ljharb commented Aug 30, 2018

AntiFish03 commented Aug 31, 2018

ljharb left a comment

AntiFish03 commented Sep 4, 2018

majapw left a comment

majapw Sep 4, 2018

majapw Sep 4, 2018

ljharb Sep 4, 2018 •

edited

Loading

AntiFish03 Sep 4, 2018

ljharb Sep 4, 2018

majapw Sep 4, 2018

majapw left a comment

AntiFish03 commented Sep 4, 2018

majapw left a comment

majapw commented Sep 12, 2018

AntiFish03 commented Sep 24, 2018

majapw commented Sep 24, 2018

AntiFish03 commented Sep 25, 2018


		export const pureComponentAvailable = typeof React.PureComponent !== 'undefined';

		export default function baseClass(pureComponent = pureComponentAvailable) {

Conditionally use PureComponent instead of Component #1335

Conditionally use PureComponent instead of Component #1335

Conversation

AntiFish03 commented Aug 30, 2018

coveralls commented Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntiFish03 commented Aug 30, 2018

AntiFish03 commented Aug 30, 2018

ljharb commented Aug 30, 2018

AntiFish03 commented Aug 30, 2018

ljharb commented Aug 30, 2018

ljharb commented Aug 30, 2018

AntiFish03 commented Aug 30, 2018

AntiFish03 commented Aug 30, 2018

ljharb commented Aug 30, 2018

AntiFish03 commented Aug 31, 2018

ljharb left a comment

Choose a reason for hiding this comment

AntiFish03 commented Sep 4, 2018

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majapw left a comment

Choose a reason for hiding this comment

AntiFish03 commented Sep 4, 2018

majapw left a comment

Choose a reason for hiding this comment

majapw commented Sep 12, 2018

AntiFish03 commented Sep 24, 2018

majapw commented Sep 24, 2018

AntiFish03 commented Sep 25, 2018

Conditionally use `PureComponent` instead of `Component` #1335

Conditionally use `PureComponent` instead of `Component` #1335

coveralls commented Aug 30, 2018 •

edited

Loading

ljharb Sep 4, 2018 •

edited

Loading