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.

Sign up for GitHub

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

Check for Node’s fs and path before trying to use them #339

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Check for Node’s fs and path before trying to use them #339

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
45b66e5
Improve test for CommonJS environment, to not be fooled by a browser …
GeoffreyBooth Jan 21, 2017
de43eb3
Actually, rather than test for Node, let’s specifically check that re…
GeoffreyBooth Jan 21, 2017
91e2307
Set the fallback value of source to an empty string, which is parseab…
GeoffreyBooth Jan 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/jison.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '';
var fs = require('fs');
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😕

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

Expand Down