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

Debug.log: remove dead code, always use console.log #507

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@jonathanperret
Contributor

jonathanperret commented Feb 25, 2016

The code to use process.stdout.write that was introduced in elm/compiler@142c2c likely never worked: the var x = x || {} trick only ever works for variables already defined in the same scope as the var statement, which process definitely is not under Node.

Additionally, console.log works just fine in Node, and adds a newline where process.stdout.write does not (which would have been immediately obvious if that code path had ever been taken).

Debug.log: remove dead code, always use console.log
The code to use `process.stdout.write` that was introduced in
elm/compiler@142c2c likely never
worked: the `var x = x || {}` trick only ever works for variables
already defined in the same scope as the `var` statement, which
`process` definitely is not under Node.

Additionally, `console.log` works just fine in Node, and adds a newline
where `process.stdout.write` does not (which would have been immediately
obvious if that code path had ever been taken).

@rtfeldman rtfeldman added the bug label Apr 21, 2017

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jul 8, 2017

Member

I don't know if this changed since the code was changed away from just calling console.log directly in the first place. Maybe it has. I am changing how logging works a bit in 0.19 so I will review it then and verify what is necessary.

Thank you for bringing this to my attention!

Member

evancz commented Jul 8, 2017

I don't know if this changed since the code was changed away from just calling console.log directly in the first place. Maybe it has. I am changing how logging works a bit in 0.19 so I will review it then and verify what is necessary.

Thank you for bringing this to my attention!

@evancz evancz closed this Jul 8, 2017

@jonathanperret

This comment has been minimized.

Show comment
Hide comment
@jonathanperret

jonathanperret Jul 9, 2017

Contributor

You did remove the problematic code in f4976fb so this PR is indeed no longer useful.

Contributor

jonathanperret commented Jul 9, 2017

You did remove the problematic code in f4976fb so this PR is indeed no longer useful.

@jonathanperret jonathanperret deleted the jonathanperret:debug-no-process-stdout branch Jul 9, 2017

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