-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Comments
Why would you want another module to modify your global scope? Use Anyway, the |
|
The following experiment was conducted with node v0.4.11. Suppose I have a file
Let's fire up node's REPL:
Alright, that's not unexpected, but now let's try
Huh ... so Now, I'll kill the REPL process and start a new one and try
Well, that looks promising, but let's try passing
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". |
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 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. |
Agreed that we should we consistent with the Node REPL. I'll investigate this. |
See #1661 for a related pull request that's in the works. |
@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. |
@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! |
@TrevorBurnham Thanks for looking into the issue. 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. |
You could just head over to my fork, |
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 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 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. |
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. |
This is now fixed on master by simply using |
Just for the record, I am very uncomfortable with this behaviour... Modules I |
Tried a search on this and it looks like it could be related to changes introduced by Issue #1035.
File modA.coffee
File modB.coffee
Running it as a program shows that
a
was set to 7 in the global context:The definition of a is not present when using the REPL:
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]
The text was updated successfully, but these errors were encountered: