"colors" npm package does not work as expected in nesh

[danielgtaylor]

"colors" npm package does not work as expected in nesh

[danielgtaylor]

It looks like colors is using Object.defineProperty on the String.prototype. I can definitely confirm the issue here but have no idea how to go about fixing it, especially since the coffee command suffers from the same issue.

Patch to fix this is welcome, as are pointers in the right direction.

[mklement0]

The problem is twofold (details further below):

• You need to set the useGlobal option to true when you pass options to repl.start() and you then need to call vm.runInThisContext() rather than vm.runInContext() when evaluating startup code - this by itself will fix the problem for the JavaScript REPL.
• Unfortunately, to also make it work with the CoffeeScript REPL, an analogous fix is required in the CoffeeScript source.
• Even more unfortunately, the CoffeeScript REPL actually breaks without the analogous fix.

Please note: I'm relatively new to GitHub, node.js, and CoffeeScript, so I'm not 100% certain my analysis is correct, but my smoke-testing indicates that my fixes are effective. I'm happy to send you a pull request, but I thought I'd get your input first.

I suppose one approach could be:

• Fix the nesh source code and for now put in a conditional that only applies the fix when running the JavaScript REPL.
• Submit a pull request to the CoffeeScript repo providing the analogous fix and, if/when it has been released refine the conditional to apply the fix to the CoffeeScript REPL too, but only above version known to contain the fix.

Details of the required fixes:

• nesh 1.3.0
  • nesh.coffee::start(): before the line that starts with processPlugins 'preStart', insert the following line: opts.useGlobal = true
  • eval.coffee::exports.postStart(): replace vm.runInContext repl.opts.evalData, repl.context with if repl.context is global then vm.runInThisContext(repl.opts.evalData) else vm.runInContext repl.opts.evalData, repl.context
• coffee-script 1.6.3 (note: based on .js files):
  • repl.js::replDefaults."eval"(): replace return cb(null, vm.runInContext(js, context, filename)); with return cb(null, context === global ? vm.runInThisContext(js, filename) : vm.runInContext(js, context, filename));

Note that the coffee-script fix doesn't change the default of the coffee REPL to useGlobal - which probably makes sense, too - but only makes it handle the case correctly IF useGlobal is set.

[danielgtaylor]

Fantastic analysis @mklement0! Your approach seems sound and I'm generally willing to merge it, however I'd be willing to bet that @jashkenas is going to ask why useGlobal is necessary before accepting such a change to CoffeeScript. I'm not sure I understand why it fixes the issue, and why you cannot use your own context within which String.prototype is manipulated successfully. I also wonder what kind of side-effects it will have to always apply useGlobal.

If this issue is some weird side-effect of the repl module then maybe it could be fixed upstream in Node itself? Do you think it is worth opening an issue asking about this with Node before we apply the fixes within Nesh? If you'd like I can open the issue but I thought I'd give you a chance to do so first because you originally reported it - just link to this issue when creating the new one (danielgtaylor/nesh#4 in markdown).

[mklement0]

My understanding is superficial, but I'm reasoning by analogy: the node REPL does use the global context (simple test: global === module.exports.repl.context), so it seems to me that the coffee REPL should do as well.

To put your question differently: In a stand-alone REPL (as opposed to an embedded one), why wouldn't you use the global context?

One possible explanation for the discrepancy is that the node creators primarily had use of the repl module as an embedded REPL in mind, and thus defaulted useGlobal to false. If, as I suspect happened in your case, a user of the module then goes with its defaults, the result is a REPL with a non-global context - which works in most cases, but fails in the ones we're discussing here.

Therefore, I don't think there's anything to fix on the node side, but I just took the bold step of submitting a pull request to the coffee-script repo - let's see what happens.
As for side effects: I only noticed one: The tests in the coffee-script repo passed with one exception - one test assumed that reusing the same variable name from a previous test would result in a new variable, whereas with the global context variables are preserved across tests.

Note that my pull request has two distinct parts:

• changing the defaults to make the REPL use the global context
• fixing the eval overload used by the REPL to correctly deal with both cases (global and non-global context).

Given that it's possible to use the CoffeeScript REPL module programmatically too (as you do) - where you in theory get to choose between global and non-global context via options - I would expect at least the 2nd fix to be implemented.

If that's the case, you're free to decide whether nesh -c should use the global context or not. To me it makes sense that all stand-alone node.js-based REPLs should act the same and use the global context.

[danielgtaylor]

It looks like you're getting a lot of positive feedback upstream. Feel free to submit a pull request for the stuff that doesn't require upstream changes, and once the upstream changes are accepted and we get an approximate release date we can merge the Coffeescript-related changes as well.

[mklement0]

Will do, thanks.

Unfortunately, the upstream changes are not a done deal yet.

Seems like the pull request is stalled, because one of the two people who responded opposes the change on philosophical grounds. I have no idea how such a situation is normally resolved.

I will make the case for incorporating at least the mere bug fix - the one that would allow you nesh to run in the global context - and perhaps decide the defaults debate separately.

[danielgtaylor]

@mklement0 any updates on the CoffeeScript side of things?

[mklement0]

Semi-good news: the bug fix has been merged so that nesh will be able to create its CoffeeScript REPL in the global context; I presume this will be in the next CoffeeScript release, but I have no idea when that will be.

(By contrast, since @michaelficarra has reservations about switching the official, stand-alone CoffeeScript REPL to the global context, nothing has changed there.)

[michaelficarra]

For context, see jashkenas/coffeescript#3113 and jashkenas/coffeescript#3150

[matanster]

Any update on the next coffeescript release trajectory? enabling a coffeescript REPL will be helpful.

[mklement0]

@matanster Just to be clear: nesh already provides a CoffeeScript REPL (nesh -c), it's just that packages that modify prototypes don't work as expected at the moment.

The next CoffeeScript release, required to remedy this (in tandem with a nesh update), is (pardon the pun) brewing: jashkenas/coffeescript#3141 (despite what the issue title says, it sounds like the next version will be 1.7).

@danielgtaylor At this point it's reasonable to assume the fix will be in the next CS release, so if you wanted to prepare ahead of the latter, you could set the relevant line in src/languages/coffee.coffee to nesh.defaults.useGlobal = semver.satisfies(coffee.VERSION, "> 1.6.3")

[mklement0]

Daniel, I've noticed that what is tagged 1.4 here on GitHub differs from the same version in the npm repository: on GitHub the fix for making nesh's JavaScript REPL use the global context is in place, but that's not true for the npm package.

[danielgtaylor]

@mklement0 wow I'm not sure how I managed to screw that up. I've fixed the package on npm, and have done a patch release (1.4.1) so people with existing installs can upgrade and get the latest, plus the patch release enables the check you mentioned above. I've also added a warning which will hopefully make it more clear that imports modifying built-in prototypes will not work unless CoffeeScript is updated.

To everyone else: nesh-1.4.1 includes the fix for this issue, however until CoffeeScript is updated you will get a warning about the issue and packages like colors will still not work. Once CoffeeScript is updated, you can run npm install -g coffee-script to get the latest version and nesh will pick it up. After that, everything should work.

[matanster]

Thanks for noticing it 👍
@danielgtaylor, does this also somehow solve (after coffeescript is updated) nesh not having the running context available when invoked from node.js? http://stackoverflow.com/questions/12811744/how-do-i-start-a-coffeescript-repl-from-within-a-coffeescript-script

[danielgtaylor]

@matanster it should, yes. Anything in the current global context should be passed through to the REPL. It should be testable by installing CoffeeScript from git and making sure the version is updated, but I have not tested it yet. Once the CoffeeScript update is released I hope to add a unit test to ensure the behavior works as expected.

[matanster]

@danielgtaylor if I go for a REPL before CoffeeScript is updated, I may provide the validation for that myself.... it's much more useful when your REPL knows your context.... 👍 many thanks!

[mklement0]

Thanks, @danielgtaylor - looks great.

[mklement0]

Good news, @danielgtaylor: Coffee Script 1.7 has just been released and everything appears to work fine, both in CoffeeScript's own REPL and in nesh -c.

[danielgtaylor]

Awesome! Closing this issue - please reopen if any problems come up.