build: migrate to community nw-builder #1029
Conversation
ayushmanchhabra
changed the title
|
@bastimeyer This is almost done from my end. Only thing is during |
|
Motivation for this PR is to see if a project using nw-builder v3 can successfully be migrated to v4. |
There was a problem hiding this comment.
Well, thanks for the PR.
As you probably have noticed yourself, this is all very old stuff on a highly customized build process, using outdated tech by several (dozens of) generations. I therefore don't expect anyone except for myself to properly and successfully migrate the code. I can already see from the diff of your PR, at least in its current state, that it's doing a couple of things wrong and it's also missing stuff.
I'm not sure if I want to fully review and run/test this just yet, because I don't have motivation to dig into all this and check the validity of the changes. While the application is still used by lots of people today, my development interests have been elsewhere for years now, namely Streamlink. Streamlink Twitch GUI has been on life support since at least 2019 or even earlier, hence the neglection of this and build dependencies.
So when updating the build process, the top priority must be to not break anything and keep the resulting build artifacts 100% identical. This means that anything newly added to nw-builder needs to be verified, on top of the general behavior changes, and there seem to be quite a lot.
Only thing is during
runtest, the NW.js child process is not terminated and keeps running in the background.
From what it looks like, the run mode now returns a child process object. If you have a look at what the runtest task actually does, you can see that it listens for the appstart event, which was previously emitted by the returned NwBuilder instance. The attached event listener starts the test runner via the CDP bridge and also adds a close listener to the child process object (requested via the old API), in addition to the node process's exit event listener that's added earlier.
In regards to the missing things, there's a custom webpack plugin which (re-)launches NW.js whenever a build succeeds during dev or test-dev builds, so NW.js can be restarted by closing it while webpack is still running.
https://github.com/streamlink/streamlink-twitch-gui/blame/v2.5.3/build/tasks/webpack/plugins/nwjs.js
In its current state, your PR breaks running the dev build, the test runner and also the production build from what it looks like.
Here are the relevant tasks:
https://github.com/streamlink/streamlink-twitch-gui/blob/v2.5.3/build/tasks/configs/aliases.yml
# continuous dev build
yarn run grunt
# test
yarn run grunt test
# continuous test build
yarn run grunt test:dev
# production build + run build (as mentioned, unused task with broken config)
yarn run grunt build:prod
yarn run grunt run
# production build + compile final artifact
yarn run grunt release| const nw = new NwBuilder( options ); | ||
|
|
||
| nw.on( "log", grunt.log.writeln.bind( grunt.log ) ); | ||
| nw.on( "stdout", grunt.log.writeln.bind( grunt.log ) ); | ||
| nw.on( "stderr", grunt.log.writeln.bind( grunt.log ) ); | ||
|
|
||
| nw.run().then( done, grunt.fail.fatal ); |
There was a problem hiding this comment.
The run task is currently broken on master and doesn't read the options properly, which is caused by "recent" build changes where the NW.js version was split up between archs. The changes here are therefore also wrong.
It's a rather useless task, because it's not really meant for running the application during development or building. This was supposed to simply run a built application.
Co-authored-by: Sebastian Meyer <[email protected]>
Co-authored-by: Sebastian Meyer <[email protected]>
Co-authored-by: Sebastian Meyer <[email protected]>
Going into this, I had assumed that the changes I make would not be perfect. Maybe we can go platform wise and see what I'm missing. |
No description provided.