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

Different handling of globals in programs and REPL #1654

Closed
autotelicum opened this issue Sep 2, 2011 · 14 comments
Closed

Different handling of globals in programs and REPL #1654

autotelicum opened this issue Sep 2, 2011 · 14 comments

Comments

@autotelicum
Copy link

Tried a search on this and it looks like it could be related to changes introduced by Issue #1035.

File modA.coffee

global.a = 7

File modB.coffee

require './modA'
console.log a

Running it as a program shows that a was set to 7 in the global context:

coffee modB.coffee
7

The definition of a is not present when using the REPL:

coffee -r ./modA
coffee> console.log a
ReferenceError: a is not defined
[...]
coffee> require './modA'
{}
coffee> console.log a
ReferenceError: a is not defined
[...]

My expectation was that the REPL works the same way as running the program (as it did at least in 1.1.0 and I believe also in 1.1.1 - the output above is from 1.1.2 - not master if that could make a difference).

Using globals is useful in the REPL because doing experiments often does not initially involve well-structured code. The prelude in 'Smooth CoffeeScript' also uses this technique to hide details and complexity until they are explained in the book.

[Edit: fixed the version numbers I was referring to]

@michaelficarra
Copy link
Collaborator

Why would you want another module to modify your global scope? Use exports and (require './modA').a instead.

Anyway, the -r feature is the one that's broken, not the REPL. Our REPL behaves identically to the node REPL.

@satyr
Copy link
Collaborator

satyr commented Sep 2, 2011

Our REPL behaves identically to the node REPL

$ cat t.js
global.a = 7
require('repl').start()

$ node t.js
> a
7

@michaelsbradleyjr
Copy link

The following experiment was conducted with node v0.4.11. Suppose I have a file test.js that contains the following:

global.test = { g : 'global test' }

var localTestA = { a : 'local test' }

var localTestB = { b : 'another local test' }

exports.globalizeA = function () {
  global['localTestA'] = localTestA
}

exports.globalizeB = function (scope) {
  scope['localTestB'] = localTestB
}

Let's fire up node's REPL:

> var myTest = require('./test')
> test
ReferenceError: test is not defined
  at [object Context]:1:1
  at Interface.<anonymous> (repl.js:171:22)
  at Interface.emit (events.js:64:17)
  at Interface._onLine (readline.js:153:10)
  at Interface._line (readline.js:408:8)
  at Interface._ttyWrite (readline.js:585:14)
  at ReadStream.<anonymous> (readline.js:73:12)
  at ReadStream.emit (events.js:81:20)
  at ReadStream._emitKey (tty_posix.js:307:10)
  at ReadStream.onData (tty_posix.js:70:12)

Alright, that's not unexpected, but now let's try globalizeA():

> myTest.globalizeA()
> localTestA
ReferenceError: localTestA is not defined
  at [object Context]:1:1
  at Interface.<anonymous> (repl.js:171:22)
  at Interface.emit (events.js:64:17)
  at Interface._onLine (readline.js:153:10)
  at Interface._line (readline.js:408:8)
  at Interface._ttyWrite (readline.js:585:14)
  at ReadStream.<anonymous> (readline.js:73:12)
  at ReadStream.emit (events.js:81:20)
  at ReadStream._emitKey (tty_posix.js:307:10)
  at ReadStream.onData (tty_posix.js:70:12)
> .clear
Clearing context...
> localTestA
{ a: 'local test' }

Huh ... so globalizeA() did stick localTestA in the global scope but it's not visible until after I run the .clear command. The coffee REPL doesn't seem to provide access to .clear so this behavior may not have been experienced unless one is testing with node's REPL.

Now, I'll kill the REPL process and start a new one and try globalizeB():

> var myTest = require('./test')
> var x = {}
> myTest.globalizeB(x)
> x
{ localTestB: { b: 'another local test' } } 

Well, that looks promising, but let's try passing global and also this:

> myTest.globalizeB(global)
> global
{}
> myTest.globalizeB(this)
> this
{}
> localTestB
ReferenceError: localTestB is not defined
  at [object Context]:1:1
  at Interface.<anonymous> (repl.js:171:22)
  at Interface.emit (events.js:64:17)
  at Interface._onLine (readline.js:153:10)
  at Interface._line (readline.js:408:8)
  at Interface._ttyWrite (readline.js:585:14)
  at ReadStream.<anonymous> (readline.js:73:12)
  at ReadStream.emit (events.js:81:20)
  at ReadStream._emitKey (tty_posix.js:307:10)
  at ReadStream.onData (tty_posix.js:70:12)

Okay, so that's not really working out like I hoped and expected, but maybe my expectations don't match up with how node's REPL (v0.4.11) is supposed to work.

Any thoughts?

I do think what I've outlined above is relevant to this issue, which pertains to coffee's REPL but of course has node.js "under the hood".

@autotelicum
Copy link
Author

As to the question why? I gave a couple of reasons in the initial message. Then there is compatibility with existing code that isn't CommonJS enabled, playing around with that kind of code to see if it is worthwhile to even convert it to CommonJS. Samples can assume global names. It is relatively easy to substitute window with global - not so easy to qualify all the different globals with an object or namespace (there is no using declaration as with C++ namespaces).

What it boils down to: It is valid code. It worked in previous versions of CoffeeScript 1.x. It is a convenience for experimentation - the reason of existence for an interactive environment. The style police has nothing to say about it:

coffee --lint modA.coffee modB.coffee 
modA.coffee:    0 error(s), 0 warning(s)
modB.coffee:    0 error(s), 0 warning(s)

Anyway this should not be blown out of proportion, it is only about a limitation in the interactive environment. If it is intentional, then I can change the text in the book so the REPL and option -r is not mentioned. I would then replace that with a recommendation to write experimental programs in a text editor and execute them from there.

Node is just a runtime for executing server side CoffeeScript, it should not matter to CoffeeScript if node didn't even have a REPL.

@TrevorBurnham
Copy link
Collaborator

Agreed that we should we consistent with the Node REPL. I'll investigate this.

@michaelficarra
Copy link
Collaborator

See #1661 for a related pull request that's in the works.

@TrevorBurnham
Copy link
Collaborator

@autotelicum @michaelsbradleyjr My pull request at #1661 fixes this issue, but makes significant changes to the REPL's inner workings. Please pull it, test it, and express your support/concerns.

@michaelsbradleyjr
Copy link

@TrevorBurnham I cloned your CoffeeScript fork and have been banging around in the REPL, experimenting with globals and various "globalizing" methods.

It's working perfectly and I think it should be accepted into the official master because it allows development in the REPL to mirror precisely how one may write and incorporate modules apart from the REPL.

After all, the REPL is supposed to enhance application development. If working with the REPL forces one to implement shims or avoid harmless<->beneficial techniques in one's modules, then I think its value as a tool is lessened.

Thanks for researching this and coming up with a great solution!

@autotelicum
Copy link
Author

@TrevorBurnham Thanks for looking into the issue.
I have not been able to test it. I did a Clone in Mac of CoffeeScript master and ran:

curl https://github.com/jashkenas/coffee-script/pull/1661.patch | git am

Applying: Blacklisting certain globals from REPL rather than whitelisting (fixes #1654)
Applying: Reloading globals after every REPL command (actually fixes #1654)
Applying: Taking an axe to the sandbox (see discussion at #1661)
/Users/eh/projects/opensource/coffeescript/.git/rebase-apply/patch:214: trailing whitespace.

warning: 1 line adds whitespace errors.
Applying: Bumping node dependency to 0.4.0 (see discussion at #1661)
error: patch failed: package.json:9
error: package.json: patch does not apply
Patch failed at 0004 Bumping node dependency to 0.4.0 (see discussion at #1661)
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

It may take a while before I figure that out - the git man pages are not helpful. Consider proceeding without me because waiting for me to learn git just to test a patch ... may be like waiting for Godot.

@TrevorBurnham
Copy link
Collaborator

You could just head over to my fork, git clone it into a new directory, and npm install -g from there to make it your system's CoffeeScript.

@autotelicum
Copy link
Author

Re: git - To quote Inspector Clouseau after acts of extreme clumsiness and ignoring the blindingly obvious: "I know that"

I have tried the patch in what I consider normal use without any issues. Together with tab completion and color coding ... very good.

Example session invoked with coffee -r ./prelude with the objective: give me a list of all named C structs in subdirs to /usr/local/include. This is not a reusable module or even a program, just an interactive session.

globalize require 'fs'
globalize underscore
ls = (d = '.') -> map readdirSync(d), (f)->"#{d}/#{f}"
isDir = (f) -> statSync(f).isDirectory()
read = (f) -> if statSync(f).isFile() then readFileSync f, 'utf8'
files = map (filter (ls '/usr/local/include'), isDir), ls
data = map files, (v) -> map v, read
structs = []
map data, (d) -> select d.toString().split('\n'), (s) -> \
  if (m = s.match /^\s*struct\s+(\w+)/) then structs.push m[1]
uniq structs.sort()

It also allows me to take Oliver Steele's Functional (not a CommonJS module) into the REPL from the command-line with coffee -r ./samplesFunctional.coffee (see https://gist.github.com/1118737) and then use tab-completion with all the defined globals to write stuff like this that doubles only the even numbers:

map guard('2*', negate '%2'), [1..9]

The REPL is not for production applications - it is very handy for learning, experiments and one-offs. I am not advocating for the use of globals or eval but it should be a task for a lint utility to guide developers towards good style, so I am all for the patch being applied and the issue closed.

@jashkenas
Copy link
Owner

I'm a tentative +1, but I'd like to have @michaelficarra sign off on the proposed patch. Let's continue this conversation over on the actual pull request.

@TrevorBurnham
Copy link
Collaborator

This is now fixed on master by simply using global the REPL "sandbox," and it looks like Node 0.5.x has moved in the same direction. See discussion at #1661.

@michaelficarra
Copy link
Collaborator

Just for the record, I am very uncomfortable with this behaviour... Modules I require should not be able to modify my environment. That's just wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants