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_componentWillReceiveProps to componentDidUpdate or ge…

…tDerivedStateFromProps
  • Loading branch information
IAmThePan committed May 18, 2020
commit 241747cc53d268c1d8c0a3b929f42d7549ab1cf6
@@ -65,9 +65,10 @@ class Blocking extends React.Component {
/**
* Lifecycle event
*/
UNSAFE_componentWillReceiveProps(nextProps) {
this.updateBlockingClasses(nextProps);
this.updateSiteNotScanned(nextProps);
static getDerivedStateFromProps(nextProps) {
const blockingClasses = Blocking.buildBlockingClasses(nextProps).join(' ');
const disableBlocking = Blocking.computeSiteNotScanned(nextProps);
return { blockingClasses, disableBlocking };
}

/**
@@ -51,7 +51,15 @@ class BlockingHeader extends React.Component {
*/
componentDidMount() {
if (this.props.categories) {
this.updateBlockAll(this.props.categories);
const updates = BlockingHeader.updateBlockAll(this.props.categories, this.state.fromHere);
if (updates) {
const {
allBlocked,
fromHere,
filtered
} = updates;
this.setState({ allBlocked, fromHere, filtered });
}
}

if (typeof this.props.actions.updateTrackerCounts === 'function') {
@@ -63,18 +71,19 @@ class BlockingHeader extends React.Component {

/**
* Lifecycle event
* Refactor Notes:
* Refactor UNSAFE_componentWillReceiveProps using getDerivedStateFromProps
* because we are only manipulating state.
*/
UNSAFE_componentWillReceiveProps(nextProps) {
if (nextProps.categories) {
this.updateBlockAll(nextProps.categories);
}
static getDerivedStateFromProps(prevProps, prevState) {
return BlockingHeader.updateBlockAll(prevProps.categories, prevState.fromHere);
}

/**
* Set appropriate initial text ("Block All" or "Unblock All") in Blocking header
* when Blocking or Global Blocking view opens. Save 'allBlocked' property in state.
* Get appropriate initial text ("Block All" or "Unblock All") in Blocking header
* when Blocking or Global Blocking view opens. Return object to set in state.
*/
updateBlockAll(categories) {
static updateBlockAll(categories, fromHere) {
if (categories) {
let totalShown = 0;
let totalBlocked = 0;
@@ -92,14 +101,15 @@ class BlockingHeader extends React.Component {
}
});
});
if (this.state.fromHere || totalShown === totalBlocked || totalBlocked === 0) {
this.setState({
if (fromHere || totalShown === totalBlocked || totalBlocked === 0) {
return {
allBlocked: (totalShown === totalBlocked),
fromHere: false,
filtered
});
};
}
}
return null;
}

/**
@@ -47,17 +47,34 @@ class Category extends React.Component {
*/
componentDidMount() {
if (this.props.category) {
this.updateCategoryCheckbox(this.props.category);
const {
allShownBlocked,
totalShownBlocked,
} = Category.updateCategoryCheckbox(this.props.category);
this.setState({ allShownBlocked, totalShownBlocked });
}
}

/**
* Lifecycle event. When props changed we save in state new values
* to ensure correct rendering.
* Lifecycle event
*/
UNSAFE_componentWillReceiveProps(nextProps) {
this.updateCategoryExpanded(nextProps.expandAll);
this.updateCategoryCheckbox(nextProps.category);
componentDidUpdate(prevProps) {
this.updateCategoryExpanded(prevProps);
}

/**
* Lifecycle event.
*/
static getDerivedStateFromProps(prevProps) {
const {
allShownBlocked,
totalShownBlocked,
} = Category.updateCategoryCheckbox(prevProps.category);

return {
allShownBlocked,
totalShownBlocked
};
}

/**
@@ -85,7 +102,7 @@ class Category extends React.Component {
* Called in lifecycle events.
* @param {Object} category object containg the list of tracker objects
*/
updateCategoryCheckbox(category) {
static updateCategoryCheckbox(category) {
let totalShownBlocked = 0;
let allShownBlocked = true;
let shownCount = 0;
@@ -99,10 +116,10 @@ class Category extends React.Component {
});

allShownBlocked = (shownCount === totalShownBlocked);
this.setState({
return {
allShownBlocked,
totalShownBlocked,
});
};
}

/**
@@ -151,9 +168,11 @@ class Category extends React.Component {
* Called in lifecycle events.
* @param {boolean} global expanded state
*/
updateCategoryExpanded(expandAll) {
if (expandAll !== this.props.expandAll && expandAll !== this.state.isExpanded) {
this.setState({ isExpanded: expandAll });
updateCategoryExpanded(prevProps) {
if (this.props.expandAll !== prevProps.expandAll && this.props.expandAll !== this.state.isExpanded) {
this.setState({
isExpanded: this.props.expandAll
});
}
}

@@ -71,8 +71,8 @@ class Tracker extends React.Component {
/**
* Lifecycle event.
*/
UNSAFE_componentWillReceiveProps(nextProps) {
this.updateTrackerClasses(nextProps.tracker);
static getDerivedStateFromProps(nextProps) {
return Tracker.computeTrackerClasses(nextProps.tracker);
}

/**
@@ -122,10 +122,23 @@ class Tracker extends React.Component {
}

/**
* Set dynamic classes on .blocking-trk and save it in state.
* Set dynamic classes on .blocking-trk to state.
* @param {Object} tracker tracker object
*/
updateTrackerClasses(tracker) {
const {
trackerClasses,
warningImageTitle
} = Tracker.computeTrackerClasses(tracker);

this.setState({ trackerClasses, warningImageTitle });
}

/**
* Compute dynamic classes on .blocking-trk and return it as an object.
* @param {Object} tracker tracker object
*/
static computeTrackerClasses(tracker) {
const classes = [];
let updated_title = '';

@@ -155,10 +168,10 @@ class Tracker extends React.Component {
updated_title = t('panel_tracker_warning_slow_tooltip');
}

this.setState({
return {
trackerClasses: classes.join(' '),
warningImageTitle: updated_title,
});
};
}

/**
@@ -100,7 +100,7 @@ class DonutGraph extends React.Component {
/**
* Lifecycle event
*/
UNSAFE_componentWillReceiveProps(nextProps) {
componentDidUpdate(prevProps) {
const {
categories,
adBlock,
@@ -109,33 +109,33 @@ class DonutGraph extends React.Component {
renderGreyscale,
ghosteryFeatureSelect,
isSmall
} = this.props;
} = prevProps;

if (isSmall !== nextProps.isSmall ||
renderRedscale !== nextProps.renderRedscale ||
renderGreyscale !== nextProps.renderGreyscale ||
ghosteryFeatureSelect !== nextProps.ghosteryFeatureSelect
if (isSmall !== this.props.isSmall ||
renderRedscale !== this.props.renderRedscale ||
renderGreyscale !== this.props.renderGreyscale ||
ghosteryFeatureSelect !== this.props.ghosteryFeatureSelect
) {
this.prepareDonutContainer(nextProps.isSmall);
this.nextPropsDonut(nextProps);
this.prepareDonutContainer(this.props.isSmall);
this.nextPropsDonut(this.props);
return;
}

// componentWillReceiveProps gets called many times during page load as new trackers or unsafe data points are found
This conversation was marked as resolved by wlycdgr

This comment has been minimized.

@wlycdgr

wlycdgr Jun 4, 2020
Member

Method name in comment needs a matching update

This comment has been minimized.

@IAmThePan

IAmThePan Jun 11, 2020
Author Contributor

Nice catch. Updated in feature/cleanup-cleanup

This comment has been minimized.

@wlycdgr

wlycdgr Jun 15, 2020
Member

lgtm on feature/cleanup-cleanup

// so only compare tracker totals if we don't already have to redraw anyway as a result of the cheaper checks above
const trackerTotal = categories.reduce((total, category) => total + category.num_total, 0);
const nextTrackerTotal = nextProps.categories.reduce((total, category) => total + category.num_total, 0);
if (trackerTotal !== nextTrackerTotal) {
this.nextPropsDonut(nextProps);
const prevTrackerTotal = categories.reduce((total, category) => total + category.num_total, 0);
const trackerTotal = this.props.categories.reduce((total, category) => total + category.num_total, 0);
if (prevTrackerTotal !== trackerTotal) {
this.nextPropsDonut(this.props);
return;
}

if (!antiTracking.unknownTrackerCount && !nextProps.antiTracking.unknownTrackerCount
&& !adBlock.unknownTrackerCount && !nextProps.adBlock.unknownTrackerCount) { return; }
const unknownDataPoints = antiTracking.unknownTrackerCount + adBlock.unknownTrackerCount;
const nextUnknownDataPoints = nextProps.antiTracking.unknownTrackerCount + nextProps.adBlock.unknownTrackerCount;
if (unknownDataPoints !== nextUnknownDataPoints) {
this.nextPropsDonut(nextProps);
if (!antiTracking.unknownTrackerCount && !this.props.antiTracking.unknownTrackerCount
&& !adBlock.unknownTrackerCount && !this.props.adBlock.unknownTrackerCount) { return; }
const prevUnknownDataPoints = antiTracking.unknownTrackerCount + adBlock.unknownTrackerCount;
const unknownDataPoints = this.props.antiTracking.unknownTrackerCount + this.props.adBlock.unknownTrackerCount;
if (prevUnknownDataPoints !== unknownDataPoints) {
this.nextPropsDonut(this.props);
}
}

@@ -31,7 +31,7 @@ const RadioButtonGroup = (props) => {
));

const buttons = props.labels.map((label, index) => (
<div className="RadioButtonGroup__button">
<div key={label} className="RadioButtonGroup__button">
<RadioButton
checked={index === indexClicked}
handleClick={() => handleItemClick(index)}
@@ -35,10 +35,11 @@ class ToggleSlider extends React.Component {
/**
* Lifecycle event
*/
UNSAFE_componentWillReceiveProps(nextProps) {
this.setState({
checked: nextProps.isChecked,
});
static getDerivedStateFromProps(prevProps, prevState) {
if (prevState.checked !== prevProps.isChecked) {
return { checked: prevProps.isChecked };
}
return null;
}

/**
@@ -50,14 +50,19 @@ class GeneralSettings extends React.Component {
* Lifecycle event.
*/
componentDidMount() {
this.updateDbLastUpdated(this.props);
this.updateDbLastUpdated(this.props.settingsData);
}

/**
* Lifecycle event.
*/
UNSAFE_componentWillReceiveProps(nextProps) {
this.updateDbLastUpdated(nextProps);
static getDerivedStateFromProps(prevProps, prevState) {
const dbLastUpdated = GeneralSettings.getDbLastUpdated(prevProps.settingsData);

if (dbLastUpdated && dbLastUpdated !== prevState.dbLastUpdated) {
return { dbLastUpdated };
}
return null;
}

/**
@@ -67,14 +72,25 @@ class GeneralSettings extends React.Component {
this.props.actions.updateDatabase();
}

/**
* Get DB check timestamp and return it.
* @param {Object} settingsData
*/
static getDbLastUpdated(settingsData) {
const { language, bugs_last_updated } = settingsData;
moment.locale(language).toLowerCase().replace('_', '-');
const dbLastUpdated = moment(bugs_last_updated).format('LLL');
return dbLastUpdated;
}

/**
* Update DB check timestamp and save it in state.
* @param {Object} props
* @param {Object} settingsData
*/
updateDbLastUpdated(props) {
moment.locale(props.language).toLowerCase().replace('_', '-');
const dbLastUpdated = moment(props.bugs_last_updated).format('LLL');
if (this.state.dbLastUpdated !== dbLastUpdated) {
updateDbLastUpdated(settingsData) {
const dbLastUpdated = GeneralSettings.getDbLastUpdated(settingsData);

if (dbLastUpdated && dbLastUpdated !== this.state.dbLastUpdated) {
this.setState({ dbLastUpdated });
}
}
@@ -42,14 +42,14 @@ class Stats extends React.Component {
/**
* Lifecycle event
*/
UNSAFE_componentWillReceiveProps(nextProps) {
const nextPlus = this._isPlus(nextProps);
componentDidUpdate(prevProps) {
const thisPlus = this._isPlus(this.props);
if (nextPlus !== thisPlus) {
if (nextPlus) {
const prevPlus = this._isPlus(prevProps);
if (thisPlus !== prevPlus) {
if (thisPlus) {
this._init();
} else {
this.setState(this._reset(true));
this._resetState();
}
}
}
@@ -290,6 +290,10 @@ class Stats extends React.Component {
this.props.history.push('/login');
}

_resetState = () => {
this.setState(this._reset(true));
}

_reset = (demo) => {
const demoData = [
{ date: '2018-12-28', amount: 300, index: 0 },
@@ -44,8 +44,8 @@ class Subscription extends React.Component {
/**
* Lifecycle event.
*/
UNSAFE_componentWillReceiveProps = (nextProps) => {
if (!nextProps.loggedIn) {
componentDidUpdate = () => {
if (!this.props.loggedIn) {
this.props.history.push('/detail');
}
}
ProTip! Use n and p to navigate between commits in a pull request.