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

Console like in node.js, port node.js util module (contains functions… #286

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MrVoltz
Copy link
Contributor

@MrVoltz MrVoltz commented Jan 1, 2016

… required for console to work)

You can't argue that node.js (or devtools) console works better than ScriptCraft's.

Main differences:

  • functions support multiple arguments console.log("a", 1, "b") -> a 1 b
  • better handling of objects console.log({}) -> {} instead of [object Object]

No backwards compatibility breaks. As an additional value, developers can use other functions available in util.js module, like util.inherits() or type checking functions.

@walterhiggins
Copy link
Owner

I'm not comfortable with bringing in node-specific Joyent-owned code into ScriptCraft's codebase - e.g. debuglog is node-specific. Is there a cleaner way?

@djMax
Copy link

djMax commented Dec 17, 2016

The copyright in there is pretty clearly open to this usage though... What gives you pause?

@demipixel
Copy link

This is over a year old now. I'd rather have this than another year of waiting for a PR of a cleaner way, would prevent testing requiring multiple console.logs for every variable and JSON.stringify()ing all of them.

@TonyGravagno
Copy link
Collaborator

On one hand this looks like a good enhancement, and the license is clear that this usage is approved. On the other hand, I agree with @walterhiggins that there are other concerns.

First, there are Node-specific remnants in there which may require debugging. For example, the process global is Node-specific, not available in Rhino or SC. So on its own we see one obvious issue in the code.

I'm not suggesting the code is buggy or inadequate. But as-is it's simply not ready for inclusion. If we know before it's merged that there is an open question, then the maintainer(s) of this package have already accepted a development burden (of arguable size). This project already has a number of issues that could use attention. Adding another one won't help.

Second, consider maintenance. What if the code gets a nice enhancement or fix elsewhere. Does it fall to the maintainer(s) here to retrofit the change here? What if a bug is found in the integration with SC? Will issues will have to be reported to and resolved by Joyent, or will uswill be asked to just pull the module from their code? Who looks like the bad guy if that happens?

I'm just saying we don't know what else is in there without more research. Now, perhaps Walter would agree to a merge if you guys volunteer to do that research: edit down that code a bit to remove external "dependencies", test the port on your own, and document ScriptCraft-specific usages in a small test suite that exercises defined functionality. Yeah, I know, it's a hassle. And I understand that the code should function as-is, even with or without process and whatever other globals might be in there. If you're not inclined to do this, consider it's exactly the effort that you're asking someone else to do. Please help. If you do, I don't think this will be too tough to get by the boss. ;)

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

5 participants