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

Code Cleanup: Enforce Linting Rules & Update UNSAFE_ React Lifecycle Events #559

Merged
merged 33 commits into from Jun 4, 2020
Merged
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
75b0091
add linting for no-param-reassign and fix resulting linting errors
IAmThePan Apr 29, 2020
8d37082
add linting for prefer-object-spread and fix resulting linting errors
IAmThePan Apr 29, 2020
3049bd1
add linting for no-restricted-syntax and fix 1/2 of resulting errors
IAmThePan May 1, 2020
fe03ad7
add linting for no-prototype-builtins and fix resulting linting errors
IAmThePan May 4, 2020
f11bc18
add linting for class-methods-use-this and fix most resulting errors.…
IAmThePan May 5, 2020
5f3471e
finish linting for class-methods-use-this
IAmThePan May 6, 2020
d4d84dc
add linting for no-mixed-operators and fix resulting linting errors
IAmThePan May 6, 2020
17f90ed
add linting for import/prefer-default-export and fix resulting lintin…
IAmThePan May 8, 2020
a77cd1b
add linting for react/no-access-state-in-setstate and fix resulting l…
IAmThePan May 8, 2020
c31dbaf
add linting for react/jsx-props-no-spreading and fix resulting lintin…
IAmThePan May 11, 2020
3d50aff
finish linting errors for no-restricted-syntax. 1 remains: couldn't r…
IAmThePan May 12, 2020
227d012
Merge branch 'develop' into feature/update-linter
IAmThePan May 12, 2020
0988a29
Fix linting errors resulting from the merge with develop
IAmThePan May 12, 2020
b55be21
Refactor UNSAFE_componentWillMount into either constructor or compone…
IAmThePan May 14, 2020
241747c
Refactor UNSAFE_componentWillReceiveProps to componentDidUpdate or ge…
IAmThePan May 18, 2020
eb413fb
re-enable lint exception for no-prototype-builtins and revert calls b…
IAmThePan May 20, 2020
ac0ec93
add single line exception for no-restricted-syntax linting rule
IAmThePan May 20, 2020
fd954eb
add linting for react/destructuring-assignment and fix errors. ToDo: …
IAmThePan May 21, 2020
e815546
Fix minor bugs
IAmThePan May 22, 2020
ae8ffcc
Fix General Settings last updated text
IAmThePan May 22, 2020
4c56460
rework linting rule no-param-reassign to have more exceptions and par…
IAmThePan May 23, 2020
c5ef663
Remove file and line linting exceptions.
IAmThePan May 26, 2020
3ca2402
re-add linting rule react/sort-comp and fix resulting errors
IAmThePan May 26, 2020
4533fd7
remove added linting exception consistent-return and fix resulting er…
IAmThePan May 26, 2020
590aa60
remove added linting expression no-use-before-define and fix resultin…
IAmThePan May 26, 2020
ecb62e8
Fix linting error
IAmThePan May 26, 2020
f634c1d
fix minor bugs
IAmThePan May 26, 2020
ea51432
Code cleanup: fix PromoModal imports
IAmThePan May 26, 2020
75a88cb
Merge with develop. Fix resulting linting errors
IAmThePan May 26, 2020
68493d2
remove unnecessary hasOwnProperty calls after refactored for...in loops
IAmThePan May 28, 2020
6297062
Fix missing strings bug
IAmThePan May 28, 2020
9b4d16c
Fix last remaining string bug
IAmThePan May 28, 2020
2cb47a1
Merge branch 'develop' into feature/cleanup
IAmThePan Jun 4, 2020
File filter
Filter file types
Jump to
Jump to file
Failed to load files.

Always

Just for now

Refactor UNSAFE_componentWillMount into either constructor or compone…

…ntDidMount, leave notes for how decision was made.
  • Loading branch information
IAmThePan committed May 14, 2020
commit b55be217dc10b1f9db929ca84b97ef8466f82176
@@ -26,23 +26,31 @@ import { updateSummaryBlockingCount } from '../utils/blocking';
class Blocking extends React.Component {
static contextType = DynamicUIPortContext;

/**
* Refactoring UNSAFE_componentWillMount into Constructor
* Stats:
* Constructor runtime before refactor: 0.038ms
* Constructor + UNSAFE_componentWillMount runtime before refactor: 0.333ms
* Constructor runtime after refactor: 0.129ms
*
* Notes:
* calling buildBlockingClasses and computeSiteNotScanned in the constructor takes 0.018ms.
*
* Conclusion: Refactor using constructor as the added computation is minimal
*/
constructor(props) {
super(props);

this.state = {
blockingClasses: '',
disableBlocking: false,
};

// event bindings
this.handlePortMessage = this.handlePortMessage.bind(this);
}

/**
* Lifecycle event
*/
UNSAFE_componentWillMount() {
this.updateBlockingClasses(this.props);
this.updateSiteNotScanned(this.props);
const classes = Blocking.buildBlockingClasses(this.props);
const disableBlocking = Blocking.computeSiteNotScanned(this.props);

this.state = {
blockingClasses: classes.join(' '),
disableBlocking
};
}

/**
@@ -223,31 +231,55 @@ class Blocking extends React.Component {
}

/**
* Set dynamic classes on .blocking-trackers. Set state.
* Build dynamic classes on .blocking-trackers. Return classes
* @param {Object} props
*/
updateBlockingClasses(props) {
static buildBlockingClasses(props) {
const classes = [];

classes.push((props.toggle_individual_trackers) ? 'show-individual' : '');
classes.push((props.paused_blocking) ? 'paused' : '');
classes.push((props.sitePolicy) ? (props.sitePolicy === 2) ? 'trusted' : 'restricted' : '');

this.setState({ blockingClasses: classes.join(' ') });
return classes;
}

/**
* Disable controls for a site that cannot be scanned by Ghostery. Set state.
* Set dynamic classes on .blocking-trackers. Set state.
* @param {Object} props
*/
updateBlockingClasses(props) {
const classes = Blocking.buildBlockingClasses(props);
const blockingClasses = classes.join(' ');

if (this.state.blockingClasses !== blockingClasses) {
this.setState({ blockingClasses });
}
}

/**
* Compute whether a site cannot be scanned by Ghostery.
* @param {Object} props nextProps
*/
updateSiteNotScanned(props) {
static computeSiteNotScanned(props) {
const { siteNotScanned, categories } = props;
const pageUrl = props.pageUrl || '';

if (siteNotScanned || !categories || pageUrl.search('http') === -1) {
this.setState({ disableBlocking: true });
} else {
this.setState({ disableBlocking: false });
return true;
}
return false;
}

/**
* Disable controls for a site that cannot be scanned by Ghostery. Set state.
* @param {Object} props nextProps
*/
updateSiteNotScanned(props) {
const disableBlocking = Blocking.computeSiteNotScanned(props);

if (this.state.disableBlocking !== disableBlocking) {
this.setState({ disableBlocking });
}
}

@@ -29,6 +29,20 @@ import { renderKnownTrackerButtons, renderUnknownTrackerButtons } from './tracke
class Tracker extends React.Component {
static contextType = ThemeContext;

/**
* Refactoring UNSAFE_componentWillMount into Constructor
* Stats:
* Constructor runtime before refactor: 0.037ms
* Constructor + UNSAFE_componentWillMount runtime before refactor: 0.415ms
* Constructor runtime after refactor: 0.215ms
*
* Refactoring UNSAFE_componentWillMount into componentDidMount
* Stats:
* Constructor runtime after refactor: 0.020ms
* Constructor + componentDidMount runtime after refactor: 14.205ms
*
* Conclusion: Refactor using componentDidMount
*/
This conversation was marked as resolved by wlycdgr
Comment on lines +32 to +45

This comment has been minimized.

@wlycdgr

wlycdgr Jun 4, 2020
Member

I don't understand how this data demonstrates that the componentDidMount refactor method is faster. It seems like we need to compare the constructor + componentDidMount + UNSAFE_componentWillMount total times before refactor and after each of the two options.

This comment has been minimized.

@IAmThePan

IAmThePan Jun 11, 2020
Author Contributor

Update on Timings:
Constructor + componentDidMount took 14.250ms but those 2 functions don't run back to back. If I time the Constructor and componentDidMount separately I get:
constructor runtime after refactor: 0.0139ms
componentDidMount runtime after refactor: 0.070ms.

This is less than the constructor's runtime when refactoring into the constructor and leads to a faster initial render.

These comments are removed from feature/cleanup-cleanup.

This comment has been minimized.

@wlycdgr

wlycdgr Jun 11, 2020
Member

That makes sense!

constructor(props) {
super(props);
this.state = {
@@ -50,7 +64,7 @@ class Tracker extends React.Component {
/**
* Lifecycle event.
*/
UNSAFE_componentWillMount() {
componentDidMount() {
this.updateTrackerClasses(this.props.tracker);
}

@@ -22,22 +22,34 @@ import Rewards from '../containers/RewardsContainer';
* @memberOf PanelClasses
*/
class Detail extends React.Component {
/**
* Refactoring UNSAFE_componentWillMount into Constructor
* Stats:
* Constructor runtime before refactor: 0.085ms
* Constructor + UNSAFE_componentWillMount runtime before refactor: 0.345ms
* Constructor runtime after refactor: 0.163ms
*
* Refactoring UNSAFE_componentWillMount into componentDidMount
* Stats:
* Constructor runtime with no componentDidMount: 0.163ms
* Constructor runtime with componentDidMount: 0.078ms
* Constructor + componentDidMount runtime: 8.313ms
* Notes:
* Noticably slower when refactoring using componentDidMount
*
* Conclusion: Refactor using constructor
*/
constructor(props) {
super(props);

// event bindings
this.toggleExpanded = this.toggleExpanded.bind(this);
}

/**
* Lifecycle event
*/
UNSAFE_componentWillMount() {
// set default tab / route based on how we got to this view:
// did the user click the Rewards icon? Or the donut number / Detailed View tab in the header?
const location = this.props.history.location.pathname;
const location = props.history.location.pathname;
if (!location.includes('rewards')) {
this.props.history.push('/detail/blocking');
props.history.push('/detail/blocking');
}
}

@@ -32,6 +32,22 @@ import DynamicUIPortContext from '../contexts/DynamicUIPortContext';
class Settings extends React.Component {
static contextType = DynamicUIPortContext;

/**
* Refactoring UNSAFE_componentWillMount into Constructor
* Stats:
* Constructor runtime before refactor: 0.145ms
* Constructor + UNSAFE_componentWillMount runtime before refactor: 0.330ms
* Constructor runtime after refactor: 0.144ms
*
* Refactoring UNSAFE_componentWillMount into componentDidMount
* Stats:
* Constructor runtime with no componentDidMount: 0.123ms
* Constructor runtime with componentDidMount: 0.147ms
*
* Notes: Negligible difference using componentDidMount.
*
* Conclusion: Refactor using constructor to avoid re-render
*/
constructor(props) {
super(props);
this.state = {
@@ -45,15 +61,10 @@ class Settings extends React.Component {
this.showToast = this.showToast.bind(this);
this.hideToast = this.hideToast.bind(this);
this.handlePortMessage = this.handlePortMessage.bind(this);
}

/**
* Lifecycle event. Default sub view is set here.
*/
UNSAFE_componentWillMount() {
// Do not redirect to the default if we are trying to access a specific other subview
if (this.props.history[this.props.history.length - 1] === '/settings') {
this.props.history.push('/settings/globalblocking');
if (props.history[props.history.length - 1] === '/settings') {
props.history.push('/settings/globalblocking');
}
}

@@ -19,6 +19,23 @@ import moment from 'moment/min/moment-with-locales.min';
* @memberOf SettingsComponents
*/
class GeneralSettings extends React.Component {
/**
* Refactoring UNSAFE_componentWillMount into Constructor
* Stats:
* Constructor runtime before refactor: 0.026ms
* Constructor + UNSAFE_componentWillMount runtime before refactor: 2.410ms
* Constructor runtime after refactor: 1.631ms
*
* Refactoring UNSAFE_componentWillMount into componentDidMount
* Stats:
* Constructor runtime with no componentDidMount: 0.208ms
* Constructor runtime with componentDidMount: 0.074ms
*
* Notes:
* updateDbLastUpdated takes ~2ms to run the firt time and then 0.139ms subsequent times.
*
* Conclusion: Refactor using componentDidMount as to not do computations in the constructor
*/
This conversation was marked as resolved by wlycdgr
Comment on lines +22 to +38

This comment has been minimized.

@wlycdgr

wlycdgr Jun 4, 2020
Member

Similar to my comment above, I don't understand how some of this data is relevant to establishing the conclusion. Why isn't it sufficient to compare the runtime for the constructor-only refactor against the runtime for the componentDidMount refactor?

This comment has been minimized.

@wlycdgr

wlycdgr Jun 4, 2020
Member

More generally, we should probably remove these notes altogether once we finish the review, yeah?

This comment has been minimized.

@IAmThePan

IAmThePan Jun 11, 2020
Author Contributor

I agree that we should remove these comments. I wanted to leave them in while you all reviewed the PR, but then it got merged before I could take them out. That's why I created the feature/cleanup-cleanup branch and PR. They are removed on that branch.

This comment has been minimized.

@IAmThePan

IAmThePan Jun 11, 2020
Author Contributor

Yes, it was actually only relevant to see that UNSAFE_componentWillMount was doing calculations via #updateDbLastUpdated and that you shouldn't do calculations in the constructor.

from https://reactjs.org/docs/react-component.html#constructor:

Typically, in React constructors are only used for two purposes:

  • Initializing local state by assigning an object to this.state.
  • Binding event handler methods to an instance.

But I didn't read that until I had already done the timing for all of these refactors, so I kept it in the comments.

And an update on Timing:
I just tested the timing for the refactored constructor and compoentDidMount. I got 0.020ms for the constructor (less than before the refactor) and 0.150ms for componentDidMount. This leads to a fast initial render.

The Refactor Comments are removed in the feature/cleanup-cleanup branch and PR.

This comment has been minimized.

@wlycdgr

wlycdgr Jun 11, 2020
Member

Thank you for the detailed update/explanation & the link. Sounds good!

constructor(props) {
super(props);
this.state = {
@@ -32,7 +49,7 @@ class GeneralSettings extends React.Component {
/**
* Lifecycle event.
*/
UNSAFE_componentWillMount() {
componentDidMount() {
this.updateDbLastUpdated(this.props);
}

@@ -56,7 +73,10 @@ class GeneralSettings extends React.Component {
*/
updateDbLastUpdated(props) {
moment.locale(props.language).toLowerCase().replace('_', '-');
this.setState({ dbLastUpdated: moment(props.bugs_last_updated).format('LLL') });
const dbLastUpdated = moment(props.bugs_last_updated).format('LLL');
if (this.state.dbLastUpdated !== dbLastUpdated) {
this.setState({ dbLastUpdated });
}
}

/**
ProTip! Use n and p to navigate between commits in a pull request.