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

Fix unref error #400

Merged
merged 3 commits into from
Dec 18, 2019
Merged

Fix unref error #400

merged 3 commits into from
Dec 18, 2019

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Dec 18, 2019

See #399

Copy link
Collaborator

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR @mifi! 🎉

This looks good to me (except for one small detail)!

lib/kill.js Show resolved Hide resolved
Co-Authored-By: ehmicky <[email protected]>
@ehmicky
Copy link
Collaborator

ehmicky commented Dec 18, 2019

Waiting for @sindresorhus to review it as well.

@sindresorhus
Copy link
Owner

Would be good to open an issue on Electron about this and add a code comment that links to that issue.

@ehmicky
Copy link
Collaborator

ehmicky commented Dec 18, 2019

@sindresorhus Would it be ready to merge otherwise?

@sindresorhus
Copy link
Owner

sindresorhus commented Dec 18, 2019

Yes, I just like to give context to workarounds and ensure it will be fixed upstream eventually.

@ehmicky ehmicky merged commit 09cd28a into sindresorhus:master Dec 18, 2019
}, timeout);

// istanbul ignore else
if (t.unref) {
Copy link
Owner

Choose a reason for hiding this comment

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

@ehmicky This still needs a code comment about why it's guarded. For example:

Guarded because there's no .unref when execa is used in the renderer process in Electron.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, I thought this was ok to merge, sorry about that.
I've added the comment in #401.

@thislooksfun
Copy link

thislooksfun commented Feb 7, 2020

Is it possible to backport this fix to 3.x.x? (ref: nklayman/vue-cli-plugin-electron-builder#647 (comment))

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.

4 participants