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鈥檒l 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

Conversation

Projects
None yet
3 participants
@rahatarmanahmed
Contributor

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

馃悰 Don't load 'repl' unless needed
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

This comment has been minimized.

Contributor

zcbenz commented Jun 19, 2016

This is awesome!

@zcbenz zcbenz merged commit b74522d into electron:master Jun 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jun 20, 2016

Great find, thanks for fixing this 馃憤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment