Perform front-end dependency update (#1312) #1363
Conversation
ssciolla
added
dependencies
package.json
Outdated
| "@babel/cli": "~7.17.6", | ||
| "@babel/core": "~7.17.8", | ||
| "@babel/preset-env": "~7.16.11", | ||
| "@babel/preset-react": "~7.16.7", | ||
| "@babel/register": "~7.17.7", | ||
| "@material-ui/core": "^4.12.3", | ||
| "@material-ui/icons": "^4.11.2", | ||
| "babel-loader": "~8.2.4", | ||
| "css-loader": "~6.7.1", | ||
| "eslint-plugin-react-hooks": "^4.4.0", | ||
| "jest": "~27.5.1", | ||
| "jest-resolve": "~27.5.1", | ||
| "js-cookie": "~3.0.1", | ||
| "mini-css-extract-plugin": "2.6.0", | ||
| "react-app-polyfill": "^3.0.0", | ||
| "react-test-renderer": "17.0.2", | ||
| "standard": "^16.0.4", | ||
| "style-loader": "~3.3.1", | ||
| "typeface-roboto": "1.1.13", | ||
| "url-loader": "4.1.1", | ||
| "webpack": "5.70.x", | ||
| "webpack-bundle-tracker": "^1.4.0", | ||
| "webpack-cli": "4.9.x" |
There was a problem hiding this comment.
It looks like we could maybe optimize how node_modules is built. Some of the dependencies under devDependencies are more like dependencies (@material-ui, js-cookie, typeface-roboto?). Looking at the Docker artifacts, I see there is an effort to remove extraneous packages (npm prune --production), but I'm not sure if it's actually working. When I go to OpenShift and peak inside node_modules, I still see jest, webpack, -loader, etc. We should probably also make sure that the frontend unit test and related artifacts are left behind from the production images (currently, they are under assets/src/__tests__).
There was a problem hiding this comment.
I'd like to discuss with @jonespm but this is probably best tackled separately at a later date.
There was a problem hiding this comment.
It looks like @justin0022 originally added the npm prune afd74b5.
I've never checked to see if it's working either.
It looks like it might have to be combined with node-prune to really remove the node_modules. I saw that mentioned in this article here: https://itsopensource.com/how-to-reduce-node-docker-image-size-by-ten-times/
I think we can just address this in a separate issue but yeah having the appropriate things in devDependencies makes sense to me.
| <link rel="stylesheet" type="text/css" href="{% static 'fontawesomefree/css/all.min.css' %}"> | ||
| <script src="{% static 'fontawesomefree/js/all.min.js' %}"></script> |
There was a problem hiding this comment.
fontawesome offers a Django/solution for using icons in templates called fontawesomefree: https://pypi.org/project/fontawesomefree/ This isn't strictly a replacement for django-npm, but after we removed jquery, fontawesome seemed to be the only thing we really needed django-npm for.
To explain a bit more, fontawesome CSS classes are being used for Resources Accessed and for screenreader-only content, and thus we needed a way to load the styling. Using a strictly React approach proved somewhat challenging because of 1) we currently use string classes for icons on the Resources Accessed D3 chart, where we can't really use JSX and 2) figuring out how to load the CSS file through webpack was a headache. If D3 is ever replaced, we could look into using this approach (https://fontawesome.com/docs/web/use-with/react/), or removing fontawesome all together in favor of Material UI icons. The latter though would require configuration changes, since we currently pass CSS class strings in env.json for each resource type.
@jonespm, let me know what you think of this solution for removing django-npm. Here's the commit:
eb3492d
There was a problem hiding this comment.
I think originally pre-react we needed this for more. But that commit looks good to remove it. I'd rather have this supported package than the older unsupported one. Be great if we have time to eventually do something with D3.
|
I'm going to do more testing, so I can't guarantee this is completely stable yet, but I think we can get the review process started. @pushyamig, I'm going to request your review since you're knowledgeable about the frontend, but I understand you have other priorities too. |
is there a time I should review this by? It's been a year since I ran Myla Locally. |
Not sure, probably needs to be merged next week to have a chance of getting in the next release. I may be asked to scale it back if no one wants to or can review it soon. |
ssciolla
force-pushed
the
issue-1312-fe-dep-update
branch
from
de0c6fd
to
006ab32
Compare
|
@ssciolla I will review it today, I was able to run this locally. |
I am really not sure we need to support this. since the stuff we did back then was very basic and no further development after that. If it is too much work then I probably will skip the update. But it's upto @jonespm |
There was a problem hiding this comment.
Over all this change looks good and positive. I was able to test this with few courses both as a student and instructor view. I don't see any side effect from this change. I am approving this now, but I want fellow reviewers to test and approve as well since I am coming to project after a gap.
|
I saw a notification today that Google analytics team is sunsetting current version Universal Analytics and it is replacing with Google analytics 4. I think we are using a react-ga library which is still support Universal analytics. At some point i believe this library will update as well. I don't know what is difference between existing and new one. It looks like we need to configure this GA4 from our GA console as well. I think there may be some work from that in future |
|
@pushyamig I saw that today too. I created #1371 to capture that work that we'll need to do. We'll have to look at this for our other projects that use GA too and will still be running. I think just Student Explorer. GA in MWrite has been broken since the last switch to Google UA. |
This PR updates, pares down, and otherwise tries to improve the usage of
npmpackages in the frontend. In the process, some changes were necessary around the use of@apollo/client(mostly updating imports, configuration),react-router-dom(adjustingRouteusages),fontawesome(switching to a Python library for providing CSS styles), and@material-ui(getting rid ofRootRefinstances). A lot of different areas are touched here, but the number of non-dependency file changes is hopefully not too high. The PR aims to resolve #1312.To Do
package-lock.jsonto new lockfile format.jquery,node-sass(not used)graphql(and fix imports, course query)rc-sliderjest, checking to make sure tests still runbabel,js-cookie, loaders, etc.)RootRefusage due to deprecationreact-router-domto 5.3 (I pushed this forward a bit by adopting new version 5 patterns, but I am also waiting on updating to version 6, since they are recommending we wait until there is a backwards-compatible version. https://reactrouter.com/docs/en/v6/upgrading/v5#refactor-custom-routes).django-npmwithfontawesomefreed3updates (see Updated3#1367).