CoffeeScript support added #8

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants

I think that I have managed to add CoffeeScript support. This is my first attempt at contributing to a project on GitHub and my first attempt at altering a Node module, so I have no idea if what I have done is correct.

This should close the issue raised here #6

In the compile step source code was being read directly from the file system then concatenated into a JavaScript string. This caused problems when the source was used.

I have added a check to see if the source file ends in .coffee and if it does it will be compiled into JavaScript before being used in the concatenation.

This required the coffee-script module to be loaded into sandboxed_module.js so I have added a require('coffee-script') to this file and added coffee-script as an optional dependency in the package.json file.

I also added a very simple test that has sandboxed load a coffee script file, but doesn't do any injection, as it was the wrapping of the file contents that was the problem.

I have not updated any version numbers.

.compile(sourceToWrap, {bare: true}) is more appropriate

maybe if (~this.filename.search('.coffee$')) {

Thanks for the tips. They are now incorporated into the pull request.

@felixge felixge commented on the diff Jun 3, 2012

lib/sandboxed_module.js
@@ -1,3 +1,4 @@
+var coffeeScript = require('coffee-script');
@felixge

felixge Jun 3, 2012

Owner

So this will crash whenever coffee script is not installed?

@Honigbaum

Honigbaum Aug 13, 2012

One solution is to put coffee-script module in the dependencies. It's the way other modules like mocha go.

@felixge felixge commented on the diff Jun 3, 2012

lib/sandboxed_module.js
@@ -138,10 +139,16 @@ SandboxedModule.prototype._getCompileInfo = function() {
localValues.push(this.locals[localVariable]);
}
+ var sourceToWrap = fs.readFileSync(this.filename, 'utf8');
+
+ if (~this.filename.search('.coffee$')){
@felixge

felixge Jun 3, 2012

Owner

Bitwise not? Seriously?

@Honigbaum

Honigbaum Aug 13, 2012

Didn't see the problem. Maybe use a RegEx?

Owner

felixge commented Jun 3, 2012

Sorry for the delay, will merge if you address my two concerns in the comments.

I would be very happy about coffee-script support

@felixge felixge added a commit that referenced this pull request Sep 9, 2012

@felixge felixge Merge pull request #12 from klamping/master
Fork of CoffeeScript support Pull Request (#8)
6101c4f
Collaborator

domenic commented Oct 11, 2012

Looks like this was fixed by #12.

domenic closed this Oct 11, 2012

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