Upgrade CRA to v4, fixes associated bugs.

chrisparsons83 commented 3 years ago

What?

Updated react-scripts to v4, and the packages that are associated with create-react-app to their current versions. In the process, this PR also fixes #99 since I was cleaning out warnings and errors with the upgrade anyhow.

Why?

This unlocks the newest version of Jest, which lets everyone write better/simpler tests with react-testing-library (I was getting bugs when trying to write tests for the leaderboard fix). It also gets a bunch of console log clutter out of the way, and also gets us using React StrictMode, which should help everyone avoid render slowdowns and bugs.

How?

Okay, this is going to be long. I'm going to try and list out all the reasoning for what I've changed. If I don't cover it, it's 99% a style formatting change.

`/react/package.json`

Packages updated:

react, react-dom to v17: This gets the version caught up to current, and the big selling point is that it makes incremental changes going forward smoother if the version does fall behind. There are some event performance benefits too.
react-scripts to v4: This loads the new version of Jest for testing. It also replaces the React Hot Loader with Fast Refresh, which preserves component states while developing.
semantic-ui-react to v2: Getting off of a beta version to the current version just seems like a good idea - it cleaned up some errors too.
All the eslint-* revs: These just cleaned up errors from upgrading the other things above.

Packages added:

@testing-library stuff (jest-dom, react, user-event) - this was separated out so going forward, it needed to be included explicitly.
web-vitals - This gives data that can be loaded into things like Lighthouse in Chrome for performance measurement. It's a super tiny library (<1kb)
eslint - This needs to be explicitly added now or it'll give errors. It'll also help with code style standards.

Packages removed:

eslint-plugin-react-hooks: Per https://www.npmjs.com/package/eslint-plugin-react-hooks, since the site is on react-scripts version 4 now, the included version should be used instead.

`+ /react/src/reportWebVitals.js, - /react/src/serviceWorker.js`

Service workers weren't needed on this (and aren't needed in most apps), and they've been moved out of the standard CRA install. Instead, they've added web vitals, which is useful for helping to measure rendering performance.

`/react/src/index.js`

Besides the reference to web vitals, the other major thing that is added is React.StrictMode. You can learn more about it here: https://reactjs.org/docs/strict-mode.html - but the main thing is that it won't affect production, but does help find potential issues in development.

`/react/src/components/ADPTable.js`

Most of this is style fixing. The two things that were touched were things in #99 - the constructor which isn't necessary and threw a warning on build, and changing == to === in CheckPositionFilters(), which is just good practice in javascript because == does type coersion with the results you'd expect, and also was throwing warnings on build.

`/react/src/components/App.js`

Removed the empty ReactGA.set() because it was throwing a warning, and since the code isn't actually setting anything, it was superfluous. The examples on the ReactGA show where it could be useful, but for this code there probably isn't anything interesting to be done with it.

`/react/src/components/Helpers.js`

All that code change for just the == into === on the conditional within GetTrophy() that was throwing a warning.

`/react/src/components/LeagueRanksTable.js`

The "center" attribute on the Table isn't an actual attribute and it was throwing an error in testing.

`/react/src/components/NavBar.js`

Just like the above, Menu was given a "left" attribute which isn't an actual attribute.

`/react/src/components/Standings.js`

Changed componentWillMount() to componentDidMount() - componentWillMount is deprecated, and componentDidMount is the correct place to load AJAX calls. It also saves the number of calls, particularly if SSR is ever put in place. I also added some keys on lists to get rid of that potential rendering slowdown.

Testing?

Right now this is all hand and eye visualization that everything is still working as expected. The two tests I have in there pass again (and now throw no errors, score!)

Screenshots

This should be 100% behind the scenes and not affect the final render anywhere - it should look completely identical to the end user, just fewer console errors and errors in

Anything Else?

I'm sorry about the clutter in this PR with formatting changes, though I've looked and from what I can tell, mine is set to AirBNB's standards, which is what the project wants - at the very least I can see those single quotes are what are generally preferred and that's what my IDE wants to turn them to. In any case, it would likely (definitely) me sitting down and making a quick .prettierrc and .editorconfig file and doing one PR that's just a style fix to align with the prettierrc/editorconfig stuff is going to be helpful sooner rather than later.

sprx97 commented 3 years ago

I'll take a closer look at this and merge it this weekend when I'm home. Been a bit busy recently.

And no worries on the formatting changes -- I'm currently using the SPRX personal standard which has zero thought behind it and is probably far too C++ inspired, so happy to adopt something that works better for you. I'll continue copying whatever is used in the file when I edit it, which is basically what I do at work because some of our legacy code is older than I am :)

chrisparsons83 commented 3 years ago

100% at your leisure! It's actual hockey season, this stuff should be back burner. Feel free to ping me on questions too.

sprx97 commented 3 years ago

Merged this locally... some things still may be weird on my end because I had to use npm install --force, which doesn't seem right. But it works, so good enough.

sprx97 / OldTimeHockey