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

Remove reliance on .sh script #2

Merged
merged 4 commits into from
Apr 21, 2021
Merged

Conversation

jonasgrunert
Copy link
Contributor

First of all I want to say, that all of this is very impressive and I am very happy, that such a cool project exists.

I had some problems getting the project of the ground on my Windows system, which had to do with the bash script user in the start script. What I did was just a small hack, which replaces the bash script with a node script, which achieves the same thing.
This is mainly used for me, but maybe wone would want to integrate it nonetheless. I am open for further questions or anything, which may be required to get this to merge.

Have a great day.

This should enable cross-plattform development easily, while not requiring any special setups.
Copy link
Collaborator

@shirakaba shirakaba left a comment

Choose a reason for hiding this comment

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

Thank you very much for this (and your kind words)! Really glad to receive some expertise to solve Windows compatibility :)

I confirmed that (following my requested changes to keep macOS happy) this script exits and relaunches the app upon a HMR signal, and also ensures that the user can still read stdout and stderr from the app. If the app exits due to some other signal (such as pressing Ctrl + C to terminate the livereload script, or the signal raised by exiting the app via the quit action from Qt's menu), the live reload script terminates as expected.

So, no regressions, and fully functioning! If you could test whether my changes work on Windows too, then I'd be happy to merge them once applied!

Except for the handling of the disconnect signal all changes were valid and prove to work on windows.
@jonasgrunert
Copy link
Contributor Author

Thank you for being so fast on the review. I double checked and all changes should be implemented by now.

Forgot an s on line 7 and therefore lne 21.
A friend pointed out that the "pipe" array is the default, but "inherit" automatically uses the parents process stdio.
@shirakaba shirakaba merged commit 4301be5 into nodegui:master Apr 21, 2021
@shirakaba
Copy link
Collaborator

shirakaba commented Apr 21, 2021

Thank you for the PR! I'll add you to the contributors on svelte-nodegui itself later today.

@shirakaba
Copy link
Collaborator

Added to Svelte NodeGUI contributors in PR: nodegui/svelte-nodegui#62

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