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

Fix for #1035: Giving the eval sandbox a real Module instance #1487

Closed
wants to merge 2 commits into from
Closed

Fix for #1035: Giving the eval sandbox a real Module instance #1487

wants to merge 2 commits into from

Conversation

TrevorBurnham
Copy link
Collaborator

Read my comment at #1035 first.

This patch makes CoffeeScript's REPL behave a bit more like Node's. Node creates the module instance for its REPL like so:

var replModule = new Module('repl');

Beyond that, things get a bit tricky. require is actually defined inside of a module function and stores an internal reference to this; there's no way to get either the require function from a module or vice versa. Fortunately, require is just a one-liner:

function require(path) {
  return Module._load(path, self);
}

In this patch, it's reverse-engineered as

sandbox.require = (path) -> Module._load path, sandbox.module

Ideally we wouldn't use Node's internal API, but I don't see a good alternative...

This patch also

  1. Removes the code duplication between repl.coffee's run and coffee-script.coffee's eval. Both were defining sandbox identically, even though run always calls eval. With this patch, run just sets sandbox to {}, and eval fills in the gaps (currently by testing whether sandbox.require is defined).
  2. Makes a few other minor changes to the sandbox for consistency with Node's REPL: exports is undefined instead of {}, and __dirname and __filename can be modified.

[Edit: The issue described below was fixed by the second commit in the pull request, which further emulates the way require is instantiated by Node.]

Everything seems to run smoothly under this patch, with the exception of one notable quirk: Neither module.paths nor require.paths are defined. However, lines like

require 'coffee-script'

run fine. I'm not sure what's going on there, and being able to modify require.paths in the REPL (as it can be in Node's REPL) certainly seems desirable. Further investigation required.

breckinloggins pushed a commit to breckinloggins/coffee-script that referenced this pull request Jul 9, 2011
…verhaul of REPL and

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 jashkenas#1487. And still no REPL tests...
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.

2 participants