Fixes Node.js compilers writing to STDOUT and/or STDERR during compilation

[alexgorbatchev]

This was happening when I was trying to compiler an Ember template with Rails Assets that had deprecation warnings. The warnings would be printed to STDOUT and end up being part of the string that was going to be JSON parsed.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @arthurnn (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[eventualbuddha]

Maybe stdio instead of stdout since this is testing both stdout and stderr?

[arthurnn]

@rafaelfranca @josh @SamSaffron thoughts?

[josh]

I'd suggest process be simply undefined.

https://github.com/rails/execjs/blob/master/test/test_execjs.rb#L232-L257

[eventualbuddha]

@josh that won't work for us, in this case. Ember is using new Function('return this') to get the actual global object and getting console off that. Calling e.g. console.log then delegates to process.stdout.write, hence the approach with this PR.

[josh]

Ember is using new Function('return this') to get the actual global object and getting console off that. Calling e.g. console.log then delegates to process.stdout.write, hence the approach with this PR.

Then Ember is going to crash running in any environment that isn't node. In JSC typeof process === 'undefined', in therubyracer typeof process === 'undefined'.

[eventualbuddha]

No, it won't crash because it never references process directly. It has no need to.

1. Ember gets the global object using new Function('return this').
2. It then uses the global as context.imports exported by ember-environment :

export const context = {
  // import jQuery
  imports: originalContext.imports || global,
  // export Ember
  exports: originalContext.exports || global,
  // search for Namespaces
  lookup: originalContext.lookup || global
};

1. It gets context.imports.console in ember-console and uses it to build functions that wrap the real console functions.
2. Finally, the console methods in node delegate to stdout and stderr properties passed to the constructor, which are process.stdout and process.stderr.

In other environments console is implemented differently or not at all. If there is no console, Ember simply uses an empty function for the logging methods. This PR tries to prevent any output, regardless of whether it comes from the console object or via direct access to process. The wrapper function that shadows references to globals is not enough.

Does it make sense now?

[josh]

Ember gets the global object using new Function('return this').

If it was possible, it'd be great if new Function('return this')() was normalized across environments. But like you mentioned, shadow eval's scope.

I still think we should attempt to undefine as much of process as possible during evaluation. process.stdout.write shouldn't be a noop, but undefined.

[josh]

@eventualbuddha Does #43 work in the eval case?

[eventualbuddha]

I believe so. I'll leave it to @alexgorbatchev to check.

[alexgorbatchev]

I just tested locally, #43 also addresses this issue.

[rafaelfranca]

Closing this one since #43 was merged. Thanks for the contribution.