Check for Node’s fs and path before trying to use them #339
Conversation
…environment that happens to have `require` and `exports` defined as globals
GeoffreyBooth
changed the title
…quire(‘fs’) returns something before we try to use it
GeoffreyBooth
force-pushed
the
fix-commonjs-detection
branch
from
444563f
to
de43eb3
Compare
| @@ -1280,7 +1280,12 @@ function commonjsMain (args) { | |||
| console.log('Usage: '+args[0]+' FILE'); | |||
| process.exit(1); | |||
| } | |||
| var source = require('fs').readFileSync(require('path').normalize(args[1]), "utf8"); | |||
| var source = null; | |||
There was a problem hiding this comment.
Can exports.parser.parse(source) (a few lines below) handle source === null?
There was a problem hiding this comment.
Nope, it can't. Expected input is string, always. (... I now see you tweaked for that already.)
…le, instead of null, which is not.
| @@ -1280,7 +1280,12 @@ function commonjsMain (args) { | |||
| console.log('Usage: '+args[0]+' FILE'); | |||
| process.exit(1); | |||
| } | |||
| var source = require('fs').readFileSync(require('path').normalize(args[1]), "utf8"); | |||
| var source = null; | |||
There was a problem hiding this comment.
Nope, it can't. Expected input is string, always. (... I now see you tweaked for that already.)
| var path = require('path'); | ||
| if (typeof fs !== 'undefined' && fs !== null && typeof path !== 'undefined' && path !== null) { | ||
| source = fs.readFileSync(path.normalize(args[1]), "utf8"); | ||
| } | ||
| return exports.parser.parse(source); |
There was a problem hiding this comment.
And given that you want to code this defensively, wouldn't it be better to code it as a feature detect, i.e. along the lines of if (fs && typeof fs.readFileSync === 'function' && path && typeof path.normalize === 'function') { source = ...?
Besides, now that you start with an empty string a 'default input if fs/path don't exist in my env', which would very probably throw a parse error (on empty input) anyway, why the source = '' change? Right now you have a situation where, if readFilesync doesn't deliver, you end up with unusable input anyway (it'll always execute parser.parse("") then) so I wonder what you want to accomplish in a non-readFileSync environment. Maybe 'overload' the interface where
if (typeof args === 'string') {
// not using readFileSync, but feeding it the input straight away:
return exports.parser.parse(args);
} else {
if (readFileSync does not exist || args[] is not array) {
throw Error('barfing hairball, mentioning unexpected environment situation?');
} else {
return ...parse(source); // loaded from file: args[1], i.e. args MUST be array
}
}
That would at least make the CommonJS interface usable in both situations and add meaning to your checks; right now, all I see is a change in the way this will crash/fail. 😕
There was a problem hiding this comment.
This is getting included inside the browser-based CoffeeScript compiler. The use case for these checks is when someone is using the compiler in a project that has require.js, which basically simulates a CommonJS environment. See jashkenas/coffeescript#4391. I’m trying to make things not crash in such an environment.
Perhaps the return exports.parser.parse(source); could be inside the if, and this function returns nothing when readFileSync is unavailable; I didn’t try that. I’m under the impression though that downstream tools need whatever this function is returning, even if it’s a parsing of an empty string.
|
related: #320 |
|
There’s a workaround, per jashkenas/coffeescript#4546: var parser = require('./lib/coffeescript/grammar').parser.generate({
moduleMain: function() {}
}); |
lrGeneratorMixin.generateCommonJSModulegenerates some code that is intended to run in a CommonJS environment, within which it takes for granted that Node’sfsandpathwill be available. This may not necessarily be the case, as is demonstrated in jashkenas/coffeescript#4391. We should simply check thatrequire('fs')andrequire('path')are defined before we try to use them.