-
Notifications
You must be signed in to change notification settings - Fork 0
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
Chore/update dependencies #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks like it was very tedious; I hope IDEs were able to automate some of it for you. I can't testify how correct it all was, but the changes were internally consistent and seemed reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM--did you test that a) this builds, and b) the built artifact still functions as expected on all platforms?
@@ -27,10 +31,11 @@ | |||
"compile-prod": "cross-env ELECTRON_WEBPACK_APP_LIMS_HOST=\"aics.corp.alleninstitute.org\" ELECTRON_WEBPACK_APP_LIMS_PORT=80 NODE_ENV=production yarn compile", | |||
"build-executable": "yarn compile && yarn electron-builder", | |||
"dist": "electron-builder -p always", | |||
"test": "cross-env TS_NODE_PROJECT=tsconfig.commonjs.json TS_NODE_FILES=true NODE_ENV=production mocha src/**/test/*.{ts,tsx}", | |||
"test": "cross-env TS_NODE_PROJECT=tsconfig.commonjs.json TS_NODE_FILES=true NODE_ENV=production mocha --exit src/**/test/*.{ts,tsx}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you add --exit
because tests were hanging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort of, when the enzyme component tests break they hang so this was prevent those cases. When we move away from enzyme (hopefully soon) that shouldn't be necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what happened to Enzyme was a bummer. Was a fine library, and I don't think @testing-library
is truly fundamentally any better. Unfortunately it did kill Enzyme, necessitating us to use it. What a vulture.
package.json
Outdated
@@ -42,114 +47,102 @@ | |||
"aics" | |||
], | |||
"resolutions": { | |||
"@types/react": "16.9.2" | |||
"@types/react": "17.0.38" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it still necessary to have a resolutions
block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, removed
src/renderer/index.tsx
Outdated
import "core-js/es6/map"; | ||
import "core-js/es6/promise"; | ||
import "core-js/es6/set"; | ||
import "core-js/actual/promise"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you actually need these pony/polyfills anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't appear so, removed!
The File Upload App was long overdue for some dependency upgrades (most of them in fact), this PR upgrades them all except those blocked by
electron-webpack
(the css styling loaders essentially) due to it essentially being unmaintained it seems. We'll need to migrate to something else to replaceelectron-webpack
perhaps following in the steps of the explorer, but I left that for another PR.There were too many little changes to be done to become compatible with the newest versions to list here, but I'll note some of the bigger changes:
remote
module in electron was deprecated in favor of communicating via events throughipcMain
&ipcRenderer
. This meant some event managers needed to be created in both processes.antd
no longer supports dynamic icon specification in favor of splitting them out into individual components imported through a separate package.There are a lot of files to check here, I would recommend focusing on the build files like the package.json and webpack related files. Everything else should mostly be small changes. If time allows checking out the ipcMain and ipcRenderer usage would be good too.