Skip to content
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

#9 Upgrade Electron to v24 #164

Merged
merged 29 commits into from
Jun 23, 2023
Merged

Conversation

@@ -28,7 +27,8 @@ if (micromatch.isMatch(window.location.origin, originsAllowed) || protocolsAllow
});
});
setTimeout(() => {
ipcRenderer.sendTo(workerWebContentsId, 'bx-api-perform', channel, payload);
ipcRenderer.invoke('get-worker-contents-id')
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps store the result of this call so we just have to call it once per process.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps store the result of this call so we just have to call it once per process.

I have doubts about this suggestion
Should we create a module for this purpose?
and then use it like

  const id = require('some-new-module').workerWebContentsId

What is the right place to put it?
Is this worker-id available in any moment of time and code? What if we ask for it too early?

Copy link
Member

Choose a reason for hiding this comment

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

A module seems a little overkill, I was more thinking just storing the result of the call in a variable in the file itself

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was my first version :)
Like this

  const workerWebContentsIdPromise = ipcRenderer.invoke('get-worker-contents-id');

  const sendPerformToProxy = (channel, payload) => {
    const p = new Promise(resolve => {
      ipcRenderer.once(`bx-api-perform-response-${channel}`, (_, result) => {
        resolve(result);
      });
    });

    setTimeout(() => {
        workerWebContentsIdPromise
            .then(workerWebContentsId => ipcRenderer.sendTo(workerWebContentsId, 'bx-api-perform', channel, payload));
    }, 1);
    return p;
  };

But the same concern about requesting it too early.
And setTimeout here looks like brute workaround, so I decided not to touch it and to keep as is.

@@ -40,7 +40,8 @@ if (micromatch.isMatch(window.location.origin, originsAllowed) || protocolsAllow
});
});
setTimeout(() => {
ipcRenderer.sendTo(workerWebContentsId, 'bx-api-subscribe', channel);
ipcRenderer.invoke('get-worker-contents-id')
Copy link
Member

Choose a reason for hiding this comment

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

IDEM

app/services/main/boot.ts Show resolved Hide resolved
viktor44 and others added 2 commits June 19, 2023 09:26
@magne4000 magne4000 changed the title #9 Upgrade Electron to latest version #9 Upgrade Electron to v24 Jun 19, 2023
@magne4000
Copy link
Member

Migrated to node@18, the build version works, but yarn run dev does not, because I cannot forward --openssl-legacy-provider to the dev process (Electron forbids it). See webpack/webpack#14532

To fix that, we have multiple solutions:

  • Webpack would need monkeypatching to replace all md4 usage to xxhash64
  • Upgrade to webpack@5
  • Upgrade to electron-forge

All those solutions are not trivial. Finding a way to force/monkeypatch output.hashFunction: "xxhash64" on webpack AND all its plugins would probably be the simplest solution.

I will not have time to continue working on this for the next few weeks, so feel free to take over this subject.

@viktor44
Copy link
Collaborator Author

But I have the same test error with Node 18

 FAIL  test/jest/bang/search/test-search.ts
  ● Test suite failed to run

    1:undefined
    undefined

      at Object.reject (node_modules/@jest-runner/rpc/build/RPCProcess.js:139:18)
      at Server.emit (node_modules/event-pubsub/es5.js:74:21)
      at Server.gotData (node_modules/node-ipc/dao/socketServer.js:194:14)

hence Node upgrade won't help with this problem

@viktor44 viktor44 force-pushed the feature/electron_14 branch from 6fd440b to 430f8a9 Compare June 19, 2023 20:30
@magne4000
Copy link
Member

The test is fixed, it was due to some weird import issue 🤷‍♂️

Now only remains to fix yarn run dev.

@viktor44
Copy link
Collaborator Author

I think we should migrate to Node 18 in a separate PR
This one is quite big already

@viktor44
Copy link
Collaborator Author

I also moved your yesterday commits to separate branch
https://github.com/viktor44/station-desktop-app/tree/feature/electron_24_node_18

@magne4000
Copy link
Member

Electron 24 is tied to Node 18, so migrating one without the other is not a complete PR and will lead to unexpected behaviour.

@magne4000
Copy link
Member

I found a quick way to monkeypatch crypto usage in webpack, the PR should be all good now

@viktor44 viktor44 force-pushed the feature/electron_14 branch 2 times, most recently from 7059c7a to 415b189 Compare June 21, 2023 10:52
@magne4000
Copy link
Member

@viktor44 Good to merge on my side. Ok for you too?

@viktor44
Copy link
Collaborator Author

@viktor44 Good to merge on my side. Ok for you too?

absolutely

@viktor44 viktor44 merged commit 3fbec12 into getstation:main Jun 23, 2023
@viktor44 viktor44 deleted the feature/electron_14 branch January 11, 2024 08:30
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