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

🐛 Don't load 'repl' unless needed #6130

Merged
merged 1 commit into from Jun 19, 2016
Merged

🐛 Don't load 'repl' unless needed #6130

merged 1 commit into from Jun 19, 2016

Conversation

rahatarmanahmed
Copy link
Contributor

@rahatarmanahmed rahatarmanahmed commented Jun 19, 2016

Ever since v0.37.3, process.stdout in the renderer has not worked at all. It turns out that electron's fork of node sets stdout to a stream that does nothing if an error is thrown while creating the stream. I haven't found the root cause of the error, but guessHandleType(fd) returns 'TCP' for fd1 in v0.37.3. It used to return 'TTY' in v0.37.2.

I did a git bisect and found the first commit that had this issue. Seems like just loading the repl module is enough to cause the issue described above. This commit moves require('repl') so that it is only loaded when the --interactive flag is present.

I don't know why loading repl breaks stdout.

I'm not entirely sure how to write a test to verify that stdout is indeed writing to the correct place. It seems like the pending test checking process.stdout.isTTY is working now, though.

Fixes #5051

Git bisect revealed that process.stdout failed to initiate (and was replaced with a write stream that does nothing) when the 'repl' module was loaded. This commit moves `require('repl')` so that it is only loaded when the --interactive flag is present.

Fixes #5051
@zcbenz
Copy link
Member

zcbenz commented Jun 19, 2016

This is awesome! ✨

@zcbenz zcbenz merged commit b74522d into electron:master Jun 19, 2016
@kevinsawicki
Copy link
Contributor

Great find, thanks for fixing this 👍

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.

None yet

3 participants