Update dependencies (#173, #235) #236

ssciolla · 2021-11-08T19:50:04Z

This PR updates most project dependencies, with some changes being held back or deferred. See the tasks list below for more information. The PR aims to resolve #173 and #235.

Task list

Upgraded from node 14 (npm 6) to node 16 (npm 8). (This resulted in many changes to package-lock.json.) Contributors should switch to using node 16 locally (i.e. install using brew or other approach and update your PATH environment variable).
Upgraded TypeScript version from 4.2 to 4.4 (a couple of code changes were needed because of a change in how caught errors are typed in strict mode (unknown). See here for more info.
Migrated from @nestjs version 7 to version 8. Changes include the new use of @nestjs/axios library (where HttpModule was moved), tweaks to how rxjs was used with HttpModule due to a deprecation of .toPromise(), and a new approach to type inference of config variables and declaring they are defined when returned from ConfigService.get.
Adopt a pre-release version of @kth/canvas-api that provides native TypeScript support (allowing us to remove our custom types, yay!) (This required some adjustments to handle breaking changes related to method names, see https://github.com/KTH/canvas-api/releases/tag/v4.0.0-1).
Change some prop usage with Material UI table and Grid components in response to deprecations/interface changes.
Adjust to a typing change in papaparse (specifically Papa.unparse).
Fix docker-compose-prod.yml so wait-port is used by overriding entrypoint.
Prep client/src/index.tsx and client/src/App.tsx for later mui 5 and react-router-dom 6 changes (see https://reactrouter.com/docs/en/v6/upgrading/v5).
Remove the following packages from package.json because they are unnecessary or will be installed automatically by npm 8 as they are peer dependencies: @types/helmet (helmet has its own types), reflect-metadata (peer)
@types/sequelize (sequelize has its own types), eslint-plugin-import (peer), eslint-plugin-node (peer), and eslint-plugin-promise (peer).

Postponements

Upgrading to Material UI 5 is going to be a bigger deal, so I figured we'd do that work separately (see issue Upgrade Material UI #220).
I elected to wait on ESLint 8, because eslint-config-with-standard doesn't seem to support it yet (see Support ESLint 8 mightyiam/eslint-config-love#728).
react-router-dom 6 was just released a few days ago (as of 11/9), so I am waiting on this one. I may make the switch (which involves breaking changes) at the same time as work on Upgrade Material UI #220.

…udit fix

…CSV to work with new types(?)

… stuff

…ge usage of ConfigService to use { infer: true }

ssciolla · 2021-11-09T16:29:31Z

ccm_web/package.json

@@ -19,98 +19,93 @@
  "author": "",
  "license": "ISC",
  "dependencies": {
-    "@kth/canvas-api": "^3.0.3",
+    "@kth/canvas-api": "^3.3.0-1",


I guess this is still technically a pre-release. Should we wait to adopt this?

I decided to wait on this change, I reverted the commit, see fbece5d

Okay, because of the OAuth issue caused by an error type change, I had to change my approach here again. I am going with 4.0.0-0, which required some method name changes as well.

This reverts commit 507477d.

ssciolla · 2021-11-09T19:10:57Z

ccm_web/package.json

    "morgan": "^1.10.0",
-    "notistack": "^1.0.5",
-    "papaparse": "^5.3.0",
+    "mysql2": "^2.3.2",


mysql was already available before because of litjs, but because we're using it without ltijs, it seems like we should specify it going forward.

…uter-dom patterns in App.tsx

ssciolla · 2021-11-09T21:57:30Z

I'm opening this up for review (will probably do more testing though). Please read the notes above, as they should explain most of the changes I made (or did not make).

As an aside, I found this tool helpful for checking for new versions: https://github.com/raineorshine/npm-check-updates

pushyamig · 2021-11-10T13:19:32Z

I plan to review it sometime in the afternoon.

pushyamig · 2021-11-11T14:15:13Z

The pagination seems to be broken once you upload the csv file. I checked the with the version on the Dev instance and that is working fine.

ccm_web/client/src/components/CustomTable.tsx

…eChange

pushyamig · 2021-11-11T16:54:03Z

I still see this error nothing new we did not know about. We were hoping upgrade to the libraries might fix this. This issue is commented here #75. Should we create a new issue to handle it or we still believe that resolving #75 will fix this?

Warning: findDOMNode is deprecated in StrictMode. findDOMNode was passed an instance of Transition which is inside StrictMode. Instead, add a ref directly to the element you want to reference. Learn more about using refs safely here: https://reactjs.org/link/strict-mode-find-node

ssciolla · 2021-11-11T16:56:17Z

I still see this error nothing new we did not know about. We were hoping upgrade to the libraries might fix this. This issue is commented here #75. Should we create a new issue to handle it or we still believe that resolving #75 will fix this?
Warning: findDOMNode is deprecated in StrictMode. findDOMNode was passed an instance of Transition which is inside StrictMode. Instead, add a ref directly to the element you want to reference. Learn more about using refs safely here: https://reactjs.org/link/strict-mode-find-node

I think the best way to fix this will be to upgrade to Material UI 5. I think that version will have better support for React.StrictMode.

pushyamig · 2021-11-11T17:00:48Z

I think the best way to fix this will be to upgrade to Material UI 5. I think that version will have better support for React.StrictMode.

Upgrade to MUI-5 is in Icebox, so should we let this Warning be logged in Console log for user? Your estimate of MUI-5 upgrade is non-trival. I think we need to set the expectation to all that this Warning will happen so ignore for now. WDYT?

pushyamig · 2021-11-11T17:32:45Z

Canvas Auth flow revoking the authorization in canvas is not working. I verified it is working in CCM-Dev

Go the Canvas -> Accounts -> Settings-> Delete the authorization
Launch CCM and you will see 'CourseInfo not found'
Expected result it should redirect to Pre-Auth page in React.

If you want to get around this you need to delete the token from DB then the pre-Auth page comes in to generate the token.

ssciolla · 2021-11-11T17:51:24Z

Canvas Auth flow revoking the authorization in canvas is not working. I verified it is working in CCM-Dev
1. Go the Canvas -> Accounts -> Settings-> Delete the authorization

2. Launch CCM and you will see 'CourseInfo not found'
   Expected result it should redirect to Pre-Auth page in React.
If you want to get around this you need to delete the token from DB then the pre-Auth page comes in to generate the token.

Darn, okay, I will look into it.

pushyamig · 2021-11-11T17:55:43Z

Another test case odd behavior around Canvas Auth testing

Go To Canvas - > Developer Key -> go to YOUR_API_KEY_Config -> Undo a needed API call by CCM
You will see an error message in UI "CourseInfo doesn't exists"
Error in Logs

{"body":{"errors":[{"message":"Invalid access token."}]}

I Think it is erring out but somewhere in BE it is allowing the API call /api/course/403334 after the error.
The error should be appropriately displayed saying invalid access token and not say course info doesn't exist.

I think this might be somewhat related to this but not exactly the same #104

pushyamig · 2021-11-11T18:47:11Z

Testing the app from both prod and local mode

The FE Features are all seems to be working fine. Tested with different user by masquerading. Token was generated and things were fine.
The webpack is loading the new JS file when code changes.
Nodemon is restarting and VScode debugger is working as expected.
Swagger upgrade is good
DB integration is looks good with new DB and existing DB
Wait ports are working fine

Found some issue around Canvas Auth flow but if those issues are fixed then this PR is good to go. This is a Huge dependency update and it might not possible to check every single case. I looked at the major integration point. Pesky little bugs might come up and we should handle it as we go as more people test this.

… 4.0.0-0

ssciolla · 2021-11-11T22:24:26Z

@pushyamig, please try testing the OAuth workflow after these changes: 2211bbc

To elaborate on the issue, KTH introduced a new CanvasApiError object which is a thinned version of got.HTTPError. It's possible we could have stayed with the same version if we had added to the types, but KTH already has a TypeScript version in the works. I elected to update to 4.0.0-0 so as to stay ahead of the curve (I tried 3.3.0-1 but I had issues with the typing). ~~When I have a moment, I will follow up with them about their non-pre-release plans. I will also update the notes above related to this.~~ I followed up with the KTH team about the typing issue I found, and I also asked when they plan to release 4.0.0.

pushyamig

Looks Good!

ccm_web/client/src/components/CustomTable.tsx

pushyamig · 2021-11-11T17:45:14Z

ccm_web/client/src/App.tsx

@@ -1,7 +1,6 @@
 import React, { useEffect, useState } from 'react'
-import { BrowserRouter as Router, Route, Switch } from 'react-router-dom'
+import { Route, Switch, useLocation } from 'react-router-dom'


Why did you removed BrowserRouter as Router and used useLocation instead. what was your thinking behind this? or this is something new with version update?

I alluded to this in the above note, this is part of changes that thereact-router-dom is recommending, using their new hooks and <Route>{children}</Route> instead of <Route render={() => {}} />. See https://reactrouter.com/docs/en/v6/upgrading/v5 and https://reacttraining.com/blog/react-router-v5-1/. It will make it easier to switch to react-router-dom 6 in the future.

ssciolla · 2021-11-12T20:46:28Z

@pushyamig, thank you for the speedy and thorough review; you caught a couple very important issues and I would have been disappointed (in myself) if I committed them to main. I looked over everything again and did a quick round of testing. I agree that it's difficult to catch everything. Please let me know if you find dependency related issues going forward, I'm happy to work on resolving them. I'm merging now!

ssciolla changed the title ~~Issue 235 dependency update~~ Update dependencies (#173, #235) Nov 8, 2021

ssciolla added 9 commits November 8, 2021 19:01

Bump NodeJS version to 16

16522dd

Update TypeScript, webpack, jest, and related dependencies; run npm a…

bd6f52d

…udit fix

Fix docker-compose-prod.yml by overriding ENTRYPOINT

713815f

Update @kth/canvas-api; remove custom types file

507477d

Update a few miscellaneous libraries; modify FileParserWrapper.create…

a52b66a

…CSV to work with new types(?)

Update eslint-related dependencies (not 8 yet); update @testing-library…

7701e58

… stuff

Update database-related dependencies

65282eb

Migrate to NestJS version 8; update a couple other dependencies; chan…

3f71c5f

…ge usage of ConfigService to use { infer: true }

Update a couple more dependencies again

9819bf0

ssciolla force-pushed the issue-235-dependency-update branch from 4f25549 to 9819bf0 Compare November 9, 2021 00:01

ssciolla commented Nov 9, 2021

View reviewed changes

ssciolla added 3 commits November 9, 2021 12:10

Update @material-ui, webpack versions

6e0ba55

Revert "Update @kth/canvas-api; remove custom types file"

fbece5d

This reverts commit 507477d.

Update notistack to latest version supporting material-ui v4

4fbc872

ssciolla commented Nov 9, 2021

View reviewed changes

ssciolla added 5 commits November 9, 2021 14:19

Use latest full release for @kth/canvas-api

d3daa81

Bump nodemon version

2fb6d24

Switch from justify to justifyContent prop on Grid

47f3f3b

Move BrowserRouter, SnackbarProvider to index.tsx; use newer react-ro…

0b454d6

…uter-dom patterns in App.tsx

Bump webpack again

3ac50b9

ssciolla marked this pull request as ready for review November 9, 2021 21:53

ssciolla requested review from pushyamig and lsloan November 9, 2021 21:54

ssciolla linked an issue Nov 10, 2021 that may be closed by this pull request

Update NodeJS, TypeScript, npm packages #235

Closed

pushyamig reviewed Nov 11, 2021

View reviewed changes

ccm_web/client/src/components/CustomTable.tsx Show resolved Hide resolved

Change prop name in TablePaginationActions from onChangePage to onPag…

12f2efa

…eChange

Fix change in error type issue by changing @kth/canvas-api version to…

2211bbc

… 4.0.0-0

pushyamig approved these changes Nov 12, 2021

View reviewed changes

ssciolla added 2 commits November 12, 2021 15:25

Indent JSX block

3355e78

Bump a couple packages again

1e139cb

ssciolla merged commit 9111caf into tl-its-umich-edu:main Nov 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update dependencies (#173, #235) #236

Update dependencies (#173, #235) #236

ssciolla commented Nov 8, 2021 •

edited

ssciolla Nov 9, 2021

ssciolla Nov 9, 2021

ssciolla Nov 11, 2021

ssciolla Nov 9, 2021

ssciolla commented Nov 9, 2021 •

edited

pushyamig commented Nov 10, 2021

pushyamig commented Nov 11, 2021

pushyamig commented Nov 11, 2021

ssciolla commented Nov 11, 2021

pushyamig commented Nov 11, 2021 •

edited

pushyamig commented Nov 11, 2021

ssciolla commented Nov 11, 2021

pushyamig commented Nov 11, 2021

pushyamig commented Nov 11, 2021

ssciolla commented Nov 11, 2021 •

edited

pushyamig left a comment

pushyamig Nov 11, 2021

ssciolla Nov 12, 2021 •

edited

ssciolla commented Nov 12, 2021

Update dependencies (#173, #235) #236

Update dependencies (#173, #235) #236

Conversation

ssciolla commented Nov 8, 2021 • edited

ssciolla Nov 9, 2021

Choose a reason for hiding this comment

ssciolla Nov 9, 2021

Choose a reason for hiding this comment

ssciolla Nov 11, 2021

Choose a reason for hiding this comment

ssciolla Nov 9, 2021

Choose a reason for hiding this comment

ssciolla commented Nov 9, 2021 • edited

pushyamig commented Nov 10, 2021

pushyamig commented Nov 11, 2021

pushyamig commented Nov 11, 2021

ssciolla commented Nov 11, 2021

pushyamig commented Nov 11, 2021 • edited

pushyamig commented Nov 11, 2021

ssciolla commented Nov 11, 2021

pushyamig commented Nov 11, 2021

pushyamig commented Nov 11, 2021

ssciolla commented Nov 11, 2021 • edited

pushyamig left a comment

Choose a reason for hiding this comment

pushyamig Nov 11, 2021

Choose a reason for hiding this comment

ssciolla Nov 12, 2021 • edited

Choose a reason for hiding this comment

ssciolla commented Nov 12, 2021

ssciolla commented Nov 8, 2021 •

edited

ssciolla commented Nov 9, 2021 •

edited

pushyamig commented Nov 11, 2021 •

edited

ssciolla commented Nov 11, 2021 •

edited

ssciolla Nov 12, 2021 •

edited