Skip to content

Commit

Permalink
Fixes #1035, #1425, and #1444: (another) overhaul of REPL and
Browse files Browse the repository at this point in the history 
CoffeeScript.eval. Instead of writing about all the changes and why I
made those decisions, I'll just answer any questions in the commit
comments, so add a commit comment if you want to question anything.
Thanks to @TrevorBurnham and @satyr for their help/contributions. Also,
closes #1487. And still no REPL tests...
  • Loading branch information
@michaelficarra
michaelficarra committed Jul 6, 2011
1 parent 18ab569 commit fff4c9c
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 55 deletions.
60 changes: 41 additions & 19 deletions lib/coffee-script.js
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 7 additions & 17 deletions lib/repl.js
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 25 additions & 12 deletions src/coffee-script.coffee
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@

fs = require 'fs'
path = require 'path'
vm = require 'vm'
{Script} = require 'vm'

This comment has been minimized.

Sign in to view

Copy link
@satyr

satyr Jul 6, 2011

Collaborator 
$ cake build:browser
built ... running browser tests:

undefined:8
ype.hasOwnProperty;g=require("fs"),j=require("path"),e=require("vm").Script,c=
                                                                    ^
TypeError: Cannot read property 'Script' of undefined
Module = require 'module'
{Lexer,RESERVED} = require './lexer'
{parser} = require './parser'

Expand Down Expand Up @@ -78,20 +79,32 @@ exports.run = (code, options) ->
# The CoffeeScript REPL uses this to run the input.
exports.eval = (code, options = {}) ->
return unless code = code.trim()
sandbox = options.sandbox
unless sandbox
sandbox =
require: require
module : { exports: {} }
sandbox[g] = global[g] for g in Object.getOwnPropertyNames global
sandbox.global = sandbox
sandbox.global.global = sandbox.global.root = sandbox.global.GLOBAL = sandbox
sandbox = Script.createContext()
sandbox.global = sandbox.root = sandbox.GLOBAL = sandbox
if options.sandbox?
if options.sandbox instanceof sandbox.constructor
sandbox = options.sandbox
else
sandbox[k] = v for own k, v of options.sandbox

This comment has been minimized.

Sign in to view

Copy link
@TrevorBurnham

TrevorBurnham Jul 7, 2011

Collaborator

This feels inconsistent: If I pass sandbox: Script.createContext(), then my sandbox will be used directly (and thus modified); but if I pass sandbox: {},then my sandbox will essentially be cloned (and not modified). Is there a (concrete) advantage to ensuring that the sandbox is from Script.createContext()? A plain object seems to work just fine. I'd suggest that we accept and modify whatever's passed as options.sandbox.

This comment has been minimized.

Sign in to view

Copy link
@michaelficarra

michaelficarra Jul 7, 2011

Author Collaborator

It needs to be a Context object to solve your -> this is global issue. See the node issue I filed about it.

This comment has been minimized.

Sign in to view

Copy link
@TrevorBurnham

TrevorBurnham Jul 7, 2011

Collaborator

Ah, interesting. Well, in that case, the only change I'd make here would be to move sandbox.global = sandbox.root = sandbox.GLOBAL = sandbox to after the if options.sandbox? clause. Otherwise, you could have

sandbox = Script.createContext()
CoffeeScript.eval "global", {sandbox}  # ReferenceError

and I don't see any use case for sandboxed code having access to its global object through this but not through global.

This comment has been minimized.

Sign in to view

Copy link
@michaelficarra

michaelficarra Jul 7, 2011

Author Collaborator

I don't agree. That change would prevent the user from providing a value for global in their sandboxes, which probably isn't a good idea.

This comment has been minimized.

Sign in to view

Copy link
@TrevorBurnham

TrevorBurnham Jul 7, 2011

Collaborator

Well, why require either sandbox.module or sandbox.require to be defined, but not sandbox.global?

This comment has been minimized.

Sign in to view

Copy link
@michaelficarra

michaelficarra Jul 7, 2011

Author Collaborator

So do you think we should define module/require only when no sandbox is provided, not just when they aren't defined? That sounds like a good idea.

update: gist

This comment has been minimized.

Sign in to view

Copy link
@TrevorBurnham

TrevorBurnham Jul 7, 2011

Collaborator

Sure. But then I really want a way of getting the sandbox created by eval out of it. Otherwise, the options are just

  1. Call eval with a full-blown sandbox already in hand, or
  2. Call eval with no sandbox and have no way of accessing the globals your eval-ed code created

One option would be to change the API to have multiple return values, but it's so nice to be able to write CoffeeScript.eval code and just get a result.

I think the best alternative is to set something like options._sandbox to point to the true sandbox, whether it's options.sandbox or internally generated. It's kind of dirty to return a result through options, but I think it beats the alternatives. We could then cut out even the remaining two lines of duplicated sandbox creation code in the REPL run method, instead just reusing the options object and setting options.sandbox = options._sandbox before each call to eval.

This comment has been minimized.

Sign in to view

Copy link
@TrevorBurnham

TrevorBurnham Jul 8, 2011

Collaborator

Actually, here's a more elegant suggestion: Keep the current API of eval, but make it a trivial wrapper around another function that returns both the output and the sandbox, e.g.

exports.eval = (code, options = {}) ->
  {_, sandbox} = exports._eval code, options
  _

Then the REPL's run could call _eval instead of eval, so that it wouldn't have to duplicate the sandbox initialization code.

If you like, I can implement this and make a pull request.

This comment has been minimized.

Sign in to view

Copy link
@satyr

satyr Jul 8, 2011

Collaborator

Do we really need .eval to be a public API?

  • REPL is the only place it's used.
  • Having it hurts minification (see how require isn't mangled in extras/coffee-script.js) and possibly performance.

This comment has been minimized.

Sign in to view

Copy link
@TrevorBurnham

TrevorBurnham Jul 8, 2011

Collaborator

Yeah, I'd say that it's pretty important for CoffeeScript.eval to exist.

This comment has been minimized.

Sign in to view

Copy link
@michaelficarra

michaelficarra Jul 8, 2011

Author Collaborator

@satyr: I don't think removing CoffeeScript.eval is an option.

This comment has been minimized.

Sign in to view

Copy link
@michaelficarra

michaelficarra Jul 8, 2011

Author Collaborator

@TrevorBurnham: I don't think having an _eval would solve the problem you put forward. Users of CoffeeScript.eval would still only have their sandboxes modified when they passed a Context instance.

This comment has been minimized.

Sign in to view

Copy link
@satyr

satyr Jul 8, 2011

Collaborator

I'd say that it's pretty important for CoffeeScript.eval to exist
I don't think removing CoffeeScript.eval is an option

Reasons?

FYI, it's added exclusively for REPL (c49cd02).

This comment has been minimized.

Sign in to view

Copy link
@TrevorBurnham

TrevorBurnham Jul 8, 2011

Collaborator

@michaelficarra Right, but it provides another method that folks can use for that purpose (and which we can use for the REPL), while keeping the current behavior of eval intact. Isn't that the best we can hope for?

This comment has been minimized.

Sign in to view

Copy link
@michaelficarra

michaelficarra Jul 8, 2011

Author Collaborator

FYI, it's added exclusively for REPL (c49cd02).

That's the only place it is used internally, but we provide it in our public API so that our users have access to it. It's the same reason we have CoffeeScript.compile in our public API.

This comment has been minimized.

Sign in to view

Copy link
@satyr

satyr Jul 8, 2011

Collaborator

but we provide it in our public API so that our users have access to it. It's the same reason we have CoffeeScript.compile in our public API

While .compile is essential and consistent, neither is .eval.
You can achieve .eval via .compile (as easy as eval CoffeeScript.compile code), but the needed semantics vary across environments/use-cases (from simple eval to module._compile to Cu.evalInSandbox).
sandbox.__filename = options.filename || 'eval'
sandbox.__dirname = path.dirname sandbox.__filename
o = {}; o[k] = v for k, v of options
# define module/require only if they chose not to specify their own
unless sandbox.module or sandbox.require
Module = require 'module'
sandbox.module = _module = new Module(options.modulename || 'eval')
sandbox.require = _require = (path) -> Module._load path, _module
_module.filename = sandbox.__filename
_require[r] = require[r] for r in Object.getOwnPropertyNames require
# use the same hack node currently uses for their own REPL
_require.paths = _module.paths = Module._nodeModulePaths process.cwd()
_require.resolve = (request) -> Module._resolveFilename request, _module
o = {}
o[k] = v for own k, v of options
o.bare = on # ensure return value
js = compile "_=(#{code}\n)", o
vm.runInNewContext js, sandbox, sandbox.__filename
js = compile "(#{code}\n)", o

This comment has been minimized.

Sign in to view

Copy link
@satyr

satyr Jul 6, 2011

Collaborator 
coffee> i for i in [1..3]
3

This comment has been minimized.

Sign in to view

Copy link
@TrevorBurnham

TrevorBurnham Jul 6, 2011

Collaborator

Good catch, satyr. Michael, any particular reason for changing "_=(#{code}\n)" to "(#{code}\n)"?

This comment has been minimized.

Sign in to view

Copy link
@michaelficarra

michaelficarra Jul 6, 2011

Author Collaborator

@TrevorBurnham: Yeah, _ is only supposed to be re-assigned if the return value is not undefined. So the check below should really be unless _ is undefined.

This comment has been minimized.

Sign in to view

Copy link
@TrevorBurnham

TrevorBurnham Jul 6, 2011

Collaborator

Any chance of refactoring this so that all the _ = stuff is in repl.coffee instead of eval? I agree that the REPL should use _ the same way Node's does, but if I call CoffeeScript.eval directly from my code, I'd prefer to be able to use _ as an ordinary variable in that code.

This comment has been minimized.

Sign in to view

Copy link
@michaelficarra

michaelficarra Jul 6, 2011

Author Collaborator

Alright, both of the above mentioned issues should be fixed by b9c3e0e. Thanks for testing it out and looking it over.

edit: Sure, I'll look into it right now.

This comment has been minimized.

Sign in to view

Copy link
@michaelficarra

michaelficarra Jul 6, 2011

Author Collaborator

@TrevorBurnham: see 003f91d for the refactor you wanted and a much sweeter line continuation input/output.
_ = Script.runInContext js, sandbox
sandbox._ = _ if _?
_

# Instantiate a Lexer for our use here.
lexer = new Lexer
Expand Down
11 changes: 4 additions & 7 deletions src/repl.coffee
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ CoffeeScript = require './coffee-script'
readline = require 'readline'
{inspect} = require 'util'
{Script} = require 'vm'
Module = require 'module'

# REPL Setup

Expand All @@ -32,12 +33,8 @@ backlog = ''
# Attempt to evaluate the command. If there's an exception, print it out instead
# of exiting.
run = do ->
sandbox =
require: require
module : { exports: {} }
sandbox[g] = global[g] for g in Object.getOwnPropertyNames global
sandbox.global = sandbox
sandbox.global.global = sandbox.global.root = sandbox.global.GLOBAL = sandbox
sandbox = Script.createContext()
sandbox.global = sandbox.root = sandbox.GLOBAL = sandbox
(buffer) ->
code = backlog += '\n' + buffer.toString()
if code[code.length - 1] is '\\'
Expand All @@ -46,8 +43,8 @@ run = do ->
try
val = CoffeeScript.eval code, {
sandbox,
bare: on,
filename: 'repl'
modulename: 'repl'
}
unless val is undefined
process.stdout.write inspect(val, no, 2, enableColours) + '\n'
Expand Down

0 comments on commit fff4c9c

Please sign in to comment.