-
Notifications
You must be signed in to change notification settings - Fork 5
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
base: master
Are you sure you want to change the base?
Conversation
FYI for @strogonoff . This PR is still a draft and not yet ready. |
7757bfd
to
de759ab
Compare
config.optimization = { | ||
...config.optimization, | ||
moduleIds: "named", | ||
}; |
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.
Unexpected '}'.
|
||
config.optimization = { | ||
...config.optimization, | ||
moduleIds: "named", |
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.
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, |
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.
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"); |
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.
'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"); |
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.
'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
@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 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 cc @ronaldtse |
First, you can remove yarn.lock and node modules. |
|
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. |
Wait for a moment. |
First, remove node_modules and yarn.lock. |
@SmartWolf1220 what’s going on? The build is still not ready? How come the build is still failing? |
@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. |
So far I am seeing this error after running
|
@SmartWolf1220 Have you tested the steps you wrote? They may be missing the compile command… |
@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. |
For me to merge this PR it needs to not have draft status, though. |
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 |
Apparently I can remove the draft status :) |
@strogonoff , I will check the layout. |
Anyway, is it working well on your side with node 18? |
How come the lint and tests jobs are both failing here? |
Add Linux Build
Hello @strogonoff , It works well on Linux Debian too. |
Thank you @SmartWolf1220 ! @strogonoff can you please merge this? Thanks. |
@strogonoff once we merge this we can close a list of the automated Dependabot PRs. |
Thanks. |
…into upgrade-vite
Hello @strogonoff , I think the test fails because of node version. |
@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. |
So, as of today, we have this:
Status of GHA build testing
|
We can close them at any point. I don’t see them as particularly helpful. |
a0fe964
to
d0db796
Compare
resolve: { | ||
modules: [path.resolve(__dirname, "./src"), "node_modules"], | ||
extensions: [".ts", ".tsx", ".js", ".jsx"], | ||
alias: { |
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.
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"], |
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.
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"], |
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.
Label 'modules' on [ statement.
// Create a fresh config object | ||
const newConfig = { | ||
...config, | ||
resolve: { |
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.
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, |
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.
Expected '}' to match '{' from line 5 and instead saw '...'.
Missing semicolon.
|
||
.DayPicker-Day, .DayPicker-Day--disabled { | ||
.DayPicker-Day, |
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.
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; |
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.
Name of variable cutColorDark
should be written in all lowercase letters with hyphens instead of underscores
|
||
$pubColorLight: #3dcc91; | ||
$pubColorDark: #0f9960; | ||
$cutColorLight: #ffb366; |
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.
Name of variable cutColorLight
should be written in all lowercase letters with hyphens instead of underscores
|
||
$pubColorLight: #3dcc91; | ||
$pubColorDark: #0f9960; |
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.
Name of variable pubColorDark
should be written in all lowercase letters with hyphens instead of underscores
|
||
$pubColorLight: #3dcc91; |
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.
Name of variable pubColorLight
should be written in all lowercase letters with hyphens instead of underscores
@SmartWolf1220 Just to remind you, we have a possible regression regarding layout issue, see this screenshot: ![]() The update to Vite caused a new regression of missing app icon: ![]() After updating to Vite, the app built on GHA still cannot be launched on macOS: ![]() 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 :) |
update the electron to latest and migrate from webpack to vite.