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

Update Electron to latest: update webpack to vite #128

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

SmartWolf1220
Copy link

update the electron to latest and migrate from webpack to vite.

@ronaldtse ronaldtse requested a review from strogonoff November 29, 2024 13:11
@ronaldtse
Copy link
Contributor

FYI for @strogonoff . This PR is still a draft and not yet ready.

@SmartWolf1220 SmartWolf1220 marked this pull request as draft November 29, 2024 13:14
config.optimization = {
...config.optimization,
moduleIds: "named",
};
Copy link

Choose a reason for hiding this comment

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

Unexpected '}'.


config.optimization = {
...config.optimization,
moduleIds: "named",
Copy link

Choose a reason for hiding this comment

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

Expected '}' to match '{' from line 4 and instead saw ':'.
Expected an assignment or function call and instead saw an expression.
Missing semicolon.
Unrecoverable syntax error. (29% scanned).


config.optimization = {
...config.optimization,
Copy link

Choose a reason for hiding this comment

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

Expected '}' to match '{' from line 7 and instead saw '...'.
Missing semicolon.

const path = require('path');
const ThreadsPlugin = require('threads-plugin');
const path = require("path");
const ThreadsPlugin = require("threads-plugin");
Copy link

Choose a reason for hiding this comment

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

'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).

@@ -1,28 +1,30 @@
const path = require('path');
const ThreadsPlugin = require('threads-plugin');
const path = require("path");
Copy link

Choose a reason for hiding this comment

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

'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).

@strogonoff
Copy link
Collaborator

strogonoff commented Dec 9, 2024

@SmartWolf1220 Could you please list the steps needed to test & build the ITU OB Editor with your PRs, from clean slate, that you have tested?

Including any pre-requisites, such as Node version(s).

Note that I was able to run prepublish script on Coulomb lib, so that may be OK for now.

However, I am unable to run build the app itself (this repository).

Node v12 that you mentioned before will not work, because some dependencies that you updated in this PR now require Node v18. However, after switching to Node v18, while yarn install would work, the cdist script would fails with various TSC errors. Please verify.

cc @ronaldtse

@SmartWolf1220
Copy link
Author

First, you can remove yarn.lock and node modules.
Second, you can use Node 18.19.0, and yarn install.
Third, use yarn run dev.

@strogonoff
Copy link
Collaborator

strogonoff commented Dec 9, 2024

yarn run dev will not build the app, it only runs the app in dev mode. It will already be helpful and appreciated, but we must be able to build & package the app (which, e.g., the cdist script does) to deliver it to the end users.

@strogonoff
Copy link
Collaborator

strogonoff commented Dec 9, 2024

you can remove yarn.lock

If Yarn’s lockfile is outdated, please update this PR with the new lockfile that contains the required versions of the packages. I’ll pull the new lockfile and try with those versions.

@SmartWolf1220
Copy link
Author

Wait for a moment.
I am checking on my side.
I will let you know in several minutes.

@SmartWolf1220
Copy link
Author

First, remove node_modules and yarn.lock.
Second, yarn install,
Third, yarn clean.
Last, yarn run dist:win, or dist:mac, or dist:linux.

@ronaldtse
Copy link
Contributor

ronaldtse commented Dec 11, 2024

@SmartWolf1220 what’s going on? The build is still not ready? How come the build is still failing?

@strogonoff
Copy link
Collaborator

strogonoff commented Dec 11, 2024

@ronaldtse Wait a moment, let me test the latest changes on this PR since my last comment… If it builds, it builds.

However, @SmartWolf1220 keep in mind that if the lockfile is outdated, it is expected that you will commit the up-to-date lockfile with exact package versions. Otherwise you’re leaving the exact versions of packages up to fate at build time, and that’s not great for reproducible builds.

Clearing a lockfile means allowing all packages to update, which is both a security liability and can lead to broken builds. So, please, don’t ask us to delete the lockfile. If you want, you can delete and regenerate it yourself, in which case please commit the updated lockfile version.

@strogonoff
Copy link
Collaborator

First, remove node_modules and yarn.lock. Second, yarn install, Third, yarn clean. Last, yarn run dist:win, or dist:mac, or dist:linux.

So far I am seeing this error after running dist:mac, copying the .app from the .dmg, and running it:

Uncaught Exception:
Error: [HMR] Env ELECTRON_HMR_SOCKET_PATH is not set at eval (webpack-internal:///./node_modules/electron-webpack/out/electron-
main-hmr/main-hmr.js: 8:9)```

@strogonoff
Copy link
Collaborator

@SmartWolf1220 Have you tested the steps you wrote? They may be missing the compile command… dist does not compile.

@strogonoff
Copy link
Collaborator

strogonoff commented Dec 11, 2024

@SmartWolf1220 As of latest changes, after compiling the app with Coulomb version from your other PR, the .app can be launched, which is great. Removing lockfile was not necessary.

@ronaldtse I believe I got the app to successfully launch on arm64. I will re-check and merge the PR.

I guess next steps is we can see that it builds on GHA, including for Debian, and fix other blocking issues.

@strogonoff
Copy link
Collaborator

For me to merge this PR it needs to not have draft status, though.

@strogonoff
Copy link
Collaborator

I will need to go through all of the changes in this PR in order to make sure all is OK. There are a lot of stylistic changes (e.g., every single quote was changed to a double quote). Without them it would be much faster.

I don’t know whether a bunch of changes were made because of that “hound” linter. If so, it must be turned off, because we don’t want to have to review 99% stylistic changes in every PR. cc @ronaldtse

@strogonoff strogonoff marked this pull request as ready for review December 11, 2024 12:04
@strogonoff
Copy link
Collaborator

Apparently I can remove the draft status :)

@strogonoff
Copy link
Collaborator

strogonoff commented Dec 11, 2024

On the main screen, I observe what looks like somewhat broken layout. Not yet sure whether it’s a regression after this PR…

Screenshot 2024-12-11 at 20 05 36

@SmartWolf1220
Copy link
Author

@strogonoff , I will check the layout.

@SmartWolf1220
Copy link
Author

Anyway, is it working well on your side with node 18?

@ronaldtse
Copy link
Contributor

How come the lint and tests jobs are both failing here?

@SmartWolf1220
Copy link
Author

Hello @strogonoff , It works well on Linux Debian too.

@ronaldtse
Copy link
Contributor

Thank you @SmartWolf1220 !

@strogonoff can you please merge this? Thanks.

@ronaldtse
Copy link
Contributor

@strogonoff once we merge this we can close a list of the automated Dependabot PRs.

@SmartWolf1220
Copy link
Author

Thanks.

@SmartWolf1220
Copy link
Author

Hello @strogonoff , I think the test fails because of node version.

image

@strogonoff
Copy link
Collaborator

How come the lint and tests jobs are both failing here?

@ronaldtse I did not update other jobs, just the build jobs. The other jobs are not that critical right now. Probably they use a wrong Node version.

@strogonoff
Copy link
Collaborator

strogonoff commented Dec 17, 2024

So, as of today, we have this:

  • New version of Coulomb was released under “dev” tag, from test-smartwolf-changes branch
    • I will go through those changes and separate stylistic changes before merging into main branch and release NPM tag later
  • This PR on ITUOBE was updated to
    • switch to Coulomb dev version in dependencies
    • enable GHA build workflow for PR branches
    • enable Ubuntu build in GHA build

Status of GHA build testing

  • On Ubuntu just now I got the confirmation the GHA build can launch and basically function
    • IMG_5415
    • IMG_5417
    • IMG_5416
    • Same on Debian
    • I believe it’s successfully using gnome-keyring for password storage
  • On macOS it cannot launch with this error:
    • Screenshot 2024-12-17 at 16 19 11
  • On Windows: to be tested

cc @ronaldtse @SmartWolf1220

@strogonoff
Copy link
Collaborator

@strogonoff once we merge this we can close a list of the automated Dependabot PRs.

We can close them at any point. I don’t see them as particularly helpful.

resolve: {
modules: [path.resolve(__dirname, "./src"), "node_modules"],
extensions: [".ts", ".tsx", ".js", ".jsx"],
alias: {
Copy link

Choose a reason for hiding this comment

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

Expected an assignment or function call and instead saw an expression.
Missing semicolon.
Unrecoverable syntax error. (35% scanned).

...config,
resolve: {
modules: [path.resolve(__dirname, "./src"), "node_modules"],
extensions: [".ts", ".tsx", ".js", ".jsx"],
Copy link

Choose a reason for hiding this comment

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

Expected '}' to match '{' from line 7 and instead saw ':'.
Expected an assignment or function call and instead saw an expression.
Missing semicolon.

const newConfig = {
...config,
resolve: {
modules: [path.resolve(__dirname, "./src"), "node_modules"],
Copy link

Choose a reason for hiding this comment

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

Label 'modules' on [ statement.

// Create a fresh config object
const newConfig = {
...config,
resolve: {
Copy link

Choose a reason for hiding this comment

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

Expected '}' to match '{' from line 3 and instead saw ':'.
Expected an assignment or function call and instead saw an expression.
Missing semicolon.

module.exports = function (config) {
// Create a fresh config object
const newConfig = {
...config,
Copy link

Choose a reason for hiding this comment

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

Expected '}' to match '{' from line 5 and instead saw '...'.
Missing semicolon.


.DayPicker-Day, .DayPicker-Day--disabled {
.DayPicker-Day,
Copy link

Choose a reason for hiding this comment

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

Selector DayPicker-Day--disabled should be written in lowercase with hyphens
Selector DayPicker-Day should be written in lowercase with hyphens
Selector should have depth of applicability no greater than 2, but was 6

$pubColorLight: #3dcc91;
$pubColorDark: #0f9960;
$cutColorLight: #ffb366;
$cutColorDark: #d9822b;
Copy link

Choose a reason for hiding this comment

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

Name of variable cutColorDark should be written in all lowercase letters with hyphens instead of underscores


$pubColorLight: #3dcc91;
$pubColorDark: #0f9960;
$cutColorLight: #ffb366;
Copy link

Choose a reason for hiding this comment

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

Name of variable cutColorLight should be written in all lowercase letters with hyphens instead of underscores


$pubColorLight: #3dcc91;
$pubColorDark: #0f9960;
Copy link

Choose a reason for hiding this comment

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

Name of variable pubColorDark should be written in all lowercase letters with hyphens instead of underscores


$pubColorLight: #3dcc91;
Copy link

Choose a reason for hiding this comment

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

Name of variable pubColorLight should be written in all lowercase letters with hyphens instead of underscores

@strogonoff
Copy link
Collaborator

strogonoff commented Dec 23, 2024

@SmartWolf1220 Just to remind you, we have a possible regression regarding layout issue, see this screenshot:

Screenshot 2024-12-11 at 20 05 36

The update to Vite caused a new regression of missing app icon:

Screenshot 2024-12-23 at 16 26 47

After updating to Vite, the app built on GHA still cannot be launched on macOS:

Screenshot 2024-12-23 at 16 29 46

We’re testing the Vite version on Linux and Windows, but I really suggest to focus on fixing the issues and postpone improvements/refactors (like switching to Vite) for later :)

@SmartWolf1220
Copy link
Author

As you can see here, it works well on my side.

image

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.

3 participants