Skip to content

Conversation

@mtgto
Copy link
Collaborator

@mtgto mtgto commented Nov 24, 2019

Objective

To use newer Electron. (#58)

This PR is very huge. Please ask me anything.

Checked:

  • build macOS app with my Apple certificate
  • test (unit test, integration test)

Not checked:

  • build Windows, Linux app
  • Database engines without SQlite3, MySQL

Changes

Update Electron to latest v7

  • Rename roles of submenu ex. hideall -> hideAll
  • Set nodeIntegration to true to use node in renderer process
  • Add hack to use setImmediate in renderer process

Update Webpack to latest v4

Update electron-builder to latest v21

Update TypeScript to latest v3.7

  • Current @types/electron contains unknown not supported in TS 2.9

Update plotly.js to latest v1.15

Update spectron to latest v9

  • Support Electron v7

- Update Electron to latest v7
  - Rename roles of submenu ex. hideall -> hideAll
  - Set nodeIntegration to true to use node in renderer process
  - Add hack to use setImmediate in renderer process
- Update Webpack to latest v4
  - Use webpack's mode argument instead of BUILD_ENV
  - Use mini-css-extract-plugin instead of outdated extracted-text-webpack plugin
- Update electron-builder to latest v21
- Update TypeScript to latest v3.7
  - Current @types/electron contains unknown not supported in TS 2.9
- Update plotly.js to latest v1.15
  - Avoid to use JSV depends on plotly.js
  - JSV contains broken package.json
    - garycourt/JSV#95
- Update spectron to latest v9
  - Support Electron v7
case UpdateState.UpdateDownloaded: {
const message = "There is an available update. Restart app to apply the latest update.";
const buttons = ["Update Now", "Later"];
dialog.showMessageBox({ message, buttons }, buttonIndex => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


if (process.platform === "darwin") {
template.unshift({
label: app.getName(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

app.getName() is deprecated.
https://electronjs.org/docs/api/app#appgetname

output: { filename: `${distDir}/main.js` },
output: {
path: distDir,
filename: `main.js`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use double quote.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.autoUpdater.checkForUpdates();
check(): Promise<UpdateCheckResult | null> {
if (isDev) return Promise.resolve(null);
return this.autoUpdater.checkForUpdates();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change is needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks correct, but you don't use a return value of this method, right?

this.check();

So I think that there is no problem with the original code.

It look good in my local.

diff --git a/src/main/updater.ts b/src/main/updater.ts
index f444f5c..0998f1b 100644
--- a/src/main/updater.ts
+++ b/src/main/updater.ts
@@ -1,4 +1,4 @@
-import { autoUpdater, AppUpdater, UpdateCheckResult } from "electron-updater";
+import { autoUpdater, AppUpdater } from "electron-updater";
 import isDev from "electron-is-dev";
 import logger from "./logger";
 
@@ -22,12 +22,12 @@ export class Updater {
     });
   }
 
-  check(): Promise<UpdateCheckResult | null> {
-    if (isDev) return Promise.resolve(null);
-    return this.autoUpdater.checkForUpdates();
+  check() {
+    if (isDev) return;
+    this.autoUpdater.checkForUpdates();
   }
 
-  async watch() {
+  watch() {
     this.check();
     setInterval(() => this.check(), WATCH_INTERVAL);
   }
$ yarn run lint
yarn run v1.19.2
$ yarn run lint:eslint && yarn run lint:tsc && yarn run lint:prettier
$ eslint --ext '.js,.jsx,.ts,.tsx' .
=============

WARNING: You are currently running a version of TypeScript which is not officially supported by typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: ~3.2.1

YOUR TYPESCRIPT VERSION: 3.7.2

Please only submit bug reports when using the officially supported version.

=============
$ tsc --noEmit
$ prettier --list-different ./**/*.{js,jsx,ts,tsx,css,json}
   Done in 12.50s.

Copy link
Collaborator Author

@mtgto mtgto Nov 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, TS 3.7 can compile original code.
But it is not good to ignore the result of method autoUpdater.checkForUpdates().
Because checkForUpdates() returns as Promise.

For example, rewrite check:

check() {
  //if (isDev) return;
  this.autoUpdater.checkForUpdates();
}
$ yarn start
yarn run v1.19.2
$ electron app/development
21:42:29.200 › Checking for update
21:42:29.235 › TypeError: Cannot read property 'catch' of undefined
    at App.<anonymous> (/Users/user/work/js/bdash/app/development/out/main.js:136:30)
    at App.emit (events.js:208:15)
GVA info: Successfully connected to the Intel plugin, offline Gen95
21:42:29.606 › Error: Error: ENOENT: no such file or directory, open '/Users/user/work/js/bdash/app/development/dev-app-update.yml'
(node:43858) UnhandledPromiseRejectionWarning: Error: ENOENT: no such file or directory, open '/Users/user/work/js/bdash/app/development/dev-app-update.yml'
(node:43858) UnhandledPromiseRejectionWarning: Error: ENOENT: no such file or directory, open '/Users/user/work/js/bdash/app/development/dev-app-update.yml'
(node:43858) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:43858) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:43858) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
(node:43858) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, my change is not enough yet.
I rollback to original code, and write it later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rollback 8938159

@hokaccha hokaccha merged commit 79f0951 into bdash-app:master Dec 13, 2019
@mtgto mtgto deleted the electron_7 branch December 22, 2019 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants