Upgrade to react-scripts v5 #695

johnclary · 2022-06-23T21:02:35Z

Associated issues

Steps I took (for reference):

$ nvm use 😉
delete /node_modules and package-lock.json
edit package.json to remove react-map-gl-draw
edit package.json to remove all devDependencies
manually set react-scripts in package.json to ^5.0.0
re-install node modules and rebuild package-lock.json: $ npm install
install react-map-gl-draw again, forcibly: $ npm install react-map-gl-draw@^0.22.3 --force
manually bump react/react-dom peerDependencies of react-map-gl-draw to ^17.x in package-lock.json
install babel import plugin: $ npm install --save-dev babel-plugin-import
remove eslintConfig in package.json and update .eslintrc.json
code changes to get file upload and components map to compile
code changes to stop exporting anonymous objects
$ npm run test ⚙️
$ npm run start ✨

About our ESLint config

It's been hard for me to untangle the effects of this upgrade on linting. We're migrating from ESLint v6 to v8, as well as upgrading a number of related parsers and configs.

Also, as far as I can tell, the react-app ESLint config was previously being ignored, because it was defined in package.json, and it was being overwritten by our .eslintrc file (docs).

One issue I immediately encountered were errors with anonymous default exports. That rule has been in place for years, but was maybe only enabled by react-app? 🤔. Anyway, I addressed those errors in this PR.

The react-app ESLint config is Create React App's base lint configuration, and they recommend that we do not ignore it. It's now enabled. Besides a few anonymous default export problems, the app is (surprisingly?) compiling without errors.

Additionally, we had a number of ESLint plugins through eslint-config-airbnb. However, none of these configs were enabled in our .eslintrc file. I removed them:

 'eslint-plugin-import': '^2.25.3',
  'eslint-plugin-jsx-a11y': '^6.5.1',
  'eslint-plugin-react': '^7.28.0',
  'eslint-plugin-react-hooks': '^4.3.0'

And, lastly, i removed the prettier linting configs as well. I don't fully understand, but the latest version threw a lot of errors and clearly had different opinions than our previous setup.

In summary, we need to revisit all of our linting soon.

Other notes

npm install will fail without our package-lock.json. we are stuck dealing with force installing react-map-gl-draw for the near future.
we're getting an ugly compilation warning about DoubleScrollbar.js:

WARNING in ./node_modules/react-double-scrollbar/dist/DoubleScrollbar.js
Module Warning (from ./node_modules/source-map-loader/dist/cjs.js):
Failed to parse source map from '/atd-moped/moped-editor/node_modules/react-double-scrollbar/dist/DoubleScrollbar.js.map' file: Error: ENOENT: no such file or directory, open '/atd-moped/moped-editor/node_modules/react-double-scrollbar/dist/DoubleScrollbar.js.map'

Seems harmless, and it's possible to suppress the warning. Discussed here.

Testing

URL to test: deploy preview and local

If testing locally

Remember to switch Node environments with$ nvm use
You will probably need to delete node_modules/ and then $ npm install, but who knows
Follow usual steps to start Moped editor + database.
Please make some code changes and test out the live-refresh.
try out the Jest tests, too: $ npm run test

Things to test

Make sure File uploads work correctly
Make sure static images load correctly:

Component map fallback image (when no components have been mapped)
The Streets / aerials buttons on the component map

Ship list

Code reviewed
Product manager approved
~~Added to QA test script if applicable~~

babel cannot find the File import from react-filepond, but it can be imported from filepond. i don't understand :/

johnclary · 2022-06-24T19:11:11Z

moped-editor/.eslintrc.json

@@ -1,4 +1,3 @@
 {
-  "extends": ["plugin:prettier/recommended"],
-  "parser": "babel-eslint"
+  "extends": ["react-app"]


see my comment in the PR description

johnclary · 2022-06-24T19:11:42Z

moped-editor/package.json

-    "eslint-plugin-react": "^7.20.3",
-    "eslint-plugin-react-hooks": "^2.5.1",
-    "prettier": "^1.19.1"
-  },


see my comment in the PR description about eslint

johnclary · 2022-06-24T19:22:00Z

moped-editor/src/utils/humanReadableFileSize.js

@@ -7,23 +7,27 @@
 *
 * @return {string} - Formatted string.
 */
-export default (bytes, si=false, dp=1) => {
-    const thresh = si ? 1000 : 1024;
+const humanReadableFileSize = (bytes, si = false, dp = 1) => {


Here is the change that fixed the ESLint warning - the rest is prettification

johnclary · 2022-06-24T19:27:05Z

moped-editor/src/views/projects/projectView/ProjectComponentsMap.css

-  background-image: url(/moped/static/images/mapAerial.jpg) !important;
-  color: white;
-}
-


it's now impossible to reference files in /public directory in CSS that lives in /src. The workaround here was to use our existing styles that we build in ProjectComponentsBaseMap.js

johnclary · 2022-06-24T19:27:42Z

moped-editor/src/views/staff/StaffListViewExportConf.js

@@ -5,38 +5,38 @@
 * @default
 */
 export const StaffListViewExportConf = {


johnclary · 2022-06-24T19:47:48Z

moped-editor/package.json

@@ -24,9 +24,6 @@
    "test": "react-scripts test",
    "eject": "react-scripts eject"
  },
-  "eslintConfig": {
-    "extends": "react-app"
-  },


The react-app ESLint config is Create React App's base lint configuration. As far as I can tell, this config was previously being ignored, because we're using an .eslintrc file which overrode the package.json config.

Docs:

If there are multiple configuration files in the same directory, ESLint will only use one. The priority order is as follows:

.eslintrc.js
.eslintrc.cjs
.eslintrc.yaml
.eslintrc.yml
.eslintrc.json
package.json

frankhereford

Thank you for this very detailed documentation via your opening PR comment. It's been exceptionally helpful both in terms of understanding the PR's contents itself, but also will serve as a useful reference for me going forward. 🙏

I've tested this both locally and on test, and I've found that everything you enumerated in your PR description works as expected:

Local
- Deployment spins up
- Live-refresh on code-change
- Jest tests 💯
Test
- File upload
- Static images referenced via CSS

Thanks for the chance to review this. I've gone through the code changes, and everything makes sense to me, but take me with a grain of salt 🧂 - I'm eager to learn from our more seasoned JS devs' reviews too.

johnclary · 2022-06-27T14:24:13Z

thanks @frankhereford!

btw, here's a diff of $ npm list against main and this branch, so that you can see all the packages that got bumped through this process.

frankhereford · 2022-06-27T14:35:18Z

@johnclary wrote:

btw, here's a diff of $ npm list against main and this branch, so that you can see all the packages that got bumped through this process.

Wow - It's a sisyphean task 🪨 and this is a huge step forward.

You got a lot of libraries (~40!!) updated or removed! You know that makes my day -- I realize this is a complex process to say the least, and I really am thankful for this PR!!

chiaberry · 2022-06-28T12:44:31Z

moped-editor/package.json

@@ -87,24 +84,15 @@
    "react-quill": "^2.0.0-beta.4",
    "react-router": "^6.3.0",
    "react-router-dom": "^6.3.0",
-    "react-scripts": "^3.4.1",
+    "react-scripts": "^5.0.0",


chiaberry

Tested locally, runs as expected. I'm leaning towards suppressing the build warning, unless theres a different fix on the horizon for the DoubleScrollbar.js.map package. I can be convinced otherwise though.

mddilley

I tested this locally and in the deploy preview, and it works great. Thanks for pushing us forward here - the hot reload is so great! Such a nice DX enhancement.

Happy to see someone getting into the lint config and cut out some of the unneeded stuff too. It will make it easier to see how to move forward there. ✨

mddilley · 2022-06-29T15:32:43Z

moped-editor/src/components/FileUpload/FileUpload.js

+import { FilePond, registerPlugin } from "react-filepond";
+// `File` imported from filepond to workaround react-scripts 5 import error
+import { File } from "filepond"


File uploads still work great in the deploy preview 👍

amenity

Project files and static images are looking good on Netlify! 👍 🚢

johnclary · 2022-07-06T14:01:14Z

thanks, y'all—here we go!

mateoclarke

👍

johnclary added 6 commits June 23, 2022 16:36

remove prettier (for now) - follow recommended eslint hooks config

7f9ceba

remove prettier (for now) - follow recommended eslint hooks config

1ddb802

this is the current lockfile

8fe5070

import File class from filepond

64abe5b

babel cannot find the File import from react-filepond, but it can be imported from filepond. i don't understand :/

use env var to construct path to /public files

459b8ae

fix fab button image styling

68192bc

johnclary changed the base branch from main to jc-react-router-dom-upgrade June 23, 2022 21:03

johnclary added 8 commits June 24, 2022 10:53

doc string

80960d2

add prettier config

04ea033

EOF newline

fb5a453

remove eslint plugins

0927c5b

remove eslint plugins

3b7b07c

add a url comment

b9c6baa

current lockfile

7eb9963

assign anon objs to vars, then export

6d63a94

johnclary commented Jun 24, 2022

View reviewed changes

johnclary requested review from mateoclarke, frankhereford, mddilley, chiaberry, tillyw and amenity June 24, 2022 20:51

johnclary changed the base branch from jc-react-router-dom-upgrade to main June 24, 2022 21:25

frankhereford approved these changes Jun 27, 2022

View reviewed changes

johnclary mentioned this pull request Jun 27, 2022

Dependabot update: ansi-html cityofaustin/atd-data-tech#8296

Closed

chiaberry reviewed Jun 28, 2022

View reviewed changes

chiaberry approved these changes Jun 28, 2022

View reviewed changes

mddilley approved these changes Jun 29, 2022

View reviewed changes

This was referenced Jun 29, 2022

Bump eventsource from 1.1.0 to 1.1.2 in /moped-editor #683

Closed

Bump async from 2.6.3 to 2.6.4 in /moped-editor #638

Closed

amenity approved these changes Jul 6, 2022

View reviewed changes

johnclary merged commit de44fe3 into main Jul 6, 2022

mateoclarke reviewed Jul 6, 2022

View reviewed changes

mateoclarke deleted the 9561-jc-react-scripts-5 branch July 6, 2022 14:57

johnclary mentioned this pull request Jul 6, 2022

Overrides react-map-gl-draw react dependency #706

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to react-scripts v5 #695

Upgrade to react-scripts v5 #695

johnclary commented Jun 23, 2022 •

edited by amenity

Loading

johnclary Jun 24, 2022

johnclary Jun 24, 2022

johnclary Jun 24, 2022

johnclary Jun 24, 2022 •

edited

Loading

johnclary Jun 24, 2022

johnclary Jun 24, 2022 •

edited

Loading

frankhereford left a comment

johnclary commented Jun 27, 2022

frankhereford commented Jun 27, 2022 •

edited

Loading

chiaberry Jun 28, 2022

chiaberry left a comment

mddilley left a comment •

edited

Loading

mddilley Jun 29, 2022

amenity left a comment

johnclary commented Jul 6, 2022

mateoclarke left a comment

Upgrade to react-scripts v5 #695

Upgrade to react-scripts v5 #695

Conversation

johnclary commented Jun 23, 2022 • edited by amenity Loading

Associated issues

Steps I took (for reference):

About our ESLint config

Other notes

Testing

If testing locally

Things to test

Ship list

johnclary Jun 24, 2022

Choose a reason for hiding this comment

johnclary Jun 24, 2022

Choose a reason for hiding this comment

johnclary Jun 24, 2022

Choose a reason for hiding this comment

johnclary Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

johnclary Jun 24, 2022

Choose a reason for hiding this comment

johnclary Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

frankhereford left a comment

Choose a reason for hiding this comment

johnclary commented Jun 27, 2022

frankhereford commented Jun 27, 2022 • edited Loading

chiaberry Jun 28, 2022

Choose a reason for hiding this comment

chiaberry left a comment

Choose a reason for hiding this comment

mddilley left a comment • edited Loading

Choose a reason for hiding this comment

mddilley Jun 29, 2022

Choose a reason for hiding this comment

amenity left a comment

Choose a reason for hiding this comment

johnclary commented Jul 6, 2022

mateoclarke left a comment

Choose a reason for hiding this comment

johnclary commented Jun 23, 2022 •

edited by amenity

Loading

johnclary Jun 24, 2022 •

edited

Loading

johnclary Jun 24, 2022 •

edited

Loading

frankhereford commented Jun 27, 2022 •

edited

Loading

mddilley left a comment •

edited

Loading