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

Cannot read property 'readFileSync' of undefined #4391

Closed
murrayju opened this issue Dec 5, 2016 · 11 comments · Fixed by #4428
Closed

Cannot read property 'readFileSync' of undefined #4391

murrayju opened this issue Dec 5, 2016 · 11 comments · Fixed by #4428

Comments

@murrayju
Copy link
Contributor

murrayju commented Dec 5, 2016

Everything works fine with CoffeeScript v1.10, but if I drop in v1.11 or newer I get this error in both the browser (using require-cs plugin) and in my r.js (requirejs) build.

Cannot read property 'readFileSync' of undefined

If I debug this in the browser (sorry for the minified code):

f._compileFile = function(c, d, f) {
    var g, k;
    null == d && (d = !1);
    null == f && (f = !1);
    k = v.readFileSync(c, "utf8");

with a breakpoint on the last line above, v is undefined.

Let me know what else I can provide to help debug this.

@GeoffreyBooth
Copy link
Collaborator

Please provide your original coffee code. Also, require('fs') shouldn't work in a browser.

@murrayju
Copy link
Contributor Author

murrayju commented Dec 6, 2016

Ok, I've made a plnkr that reproduces this:
https://embed.plnkr.co/WnoV7Q7MiB7CZ505FdAp/

Run that with the chrome debugger open, and you'll see the Uncaught TypeError that happens in coffee-script.js. I'm using the "browser-compiler" available at https://raw.githubusercontent.com/jashkenas/coffeescript/master/docs/v1/browser-compiler/coffee-script.js

I did a bit of digging, and it seems that this is only a problem when there is an Error stack trace that contains a reference to a .coffee file. Specifically, you have to evaluate e.stack to cause the uncaught TypeError to occur.

@GeoffreyBooth
Copy link
Collaborator

Error.stack is nonstandard: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Stack. I don’t know why you would be using it in production code.

@murrayju
Copy link
Contributor Author

murrayju commented Dec 7, 2016

It is not my code, it is actually in kriskowal/q. They are doing some feature detection to see if the browser supports stacks, and it is this detection code that invokes e.stack which in turn invokes your code, which throws an exception.

Anyways, I think this is a moot point. There is a code path in your "browser-compiler" build that will try to invoke require('fs'), which is obviously a bug.

In the meantime, I can probably fork q as a workaround.

@GeoffreyBooth
Copy link
Collaborator

parser.js, which is generated by jison, has the following at its end:

if (typeof require !== 'undefined' && typeof exports !== 'undefined') {
exports.parser = parser;
exports.Parser = parser.Parser;
exports.parse = function () { return parser.parse.apply(parser, arguments); };
exports.main = function commonjsMain(args) {
    if (!args[1]) {
        console.log('Usage: '+args[0]+' FILE');
        process.exit(1);
    }
    var source = require('fs').readFileSync(require('path').normalize(args[1]), "utf8");
    return exports.parser.parse(source);
};

Which is clearly supposed to only run in a CommonJS environment, but presumably your require.js setup is confusing it. I suggest you shadow require within the function that includes CoffeeScript, so that require is undefined when this code checks it.

This code isn’t part of CoffeeScript anyway, it’s jison, so it’s not in our domain to change.

@murrayju
Copy link
Contributor Author

murrayju commented Dec 8, 2016

This is also very similar to #3890. I may try to implement something similar to atom/coffee-cash#1 as a workaround.

Seems reasonable to ask that CoffeeScript's Error.prepareStackTrace should not throw exceptions.

@GeoffreyBooth
Copy link
Collaborator

Please open a ticket against jison. That’s the module generating the problematic code.

@murrayju
Copy link
Contributor Author

murrayju commented Dec 8, 2016

@GeoffreyBooth I'm not trying to be difficult here, but I find your stance frustrating. The fact that you are using jison is an implementation detail. From the user's perspective, CoffeeScript has a bug, and it is up to you guys to fix it.

Let's say that this is fixed in jison tomorrow. CoffeeScript would still have this bug at least until the new version of jison is pulled in and CoffeeScript is recompiled. The burden is still on you to track this and make sure that it gets fixed.

While I could try to open a ticket against jison, this would be difficult for me because I know nothing about jison, or how CoffeeScript is using it. It would make much more sense for you to create this ticket, since you have all of this information.

There is also a simple temporary workaround measure that could be implemented in the CoffeeScript source. Just add a try-catch around the function here: coffee-script.coffee#L352. I could make a pull request that does this if you'd like, but I don't want to waste my time if you won't merge it.

@GeoffreyBooth
Copy link
Collaborator

I would be open to accepting a PR, but I’m not familiar with that part of the codebase. It would need to be approved by @lydell or @jashkenas.

@GeoffreyBooth GeoffreyBooth reopened this Dec 9, 2016
murrayju added a commit to murrayju/coffeescript that referenced this issue Dec 9, 2016
@murrayju
Copy link
Contributor Author

murrayju commented Dec 9, 2016

@GeoffreyBooth @lydell @jashkenas see the pull request #4399. This totally solves this issue for me, but is more of a workaround. You may want to consider fixing the root of the problem (with jison?) at some point, but I think this would be fine for now.

@davibe
Copy link

davibe commented Jan 18, 2017

The "fix" removed an important feature making all stack traces useless in my backend implementations. #4418

GeoffreyBooth added a commit to GeoffreyBooth/coffeescript that referenced this issue Jan 21, 2017
…a CommonJS/Node environment, not a browser environment that may happen to have globals named `require` and `exports` (as would be the case if require.js is being used). Fixes jashkenas#4391.
GeoffreyBooth added a commit that referenced this issue Jan 22, 2017
* Revert aee27fb

* Patch Jison’s output so that it requires `fs` only if we’re truly in a CommonJS/Node environment, not a browser environment that may happen to have globals named `require` and `exports` (as would be the case if require.js is being used). Fixes #4391.

* Temporary fix for exceptions getting thrown when trying to generate a stack trace for a file that has been deleted since compilation; fixes #3890, but not well. A better solution would not try to recompile the file when trying to retrieve its stack trace.

* Save the test REPL history in the system temp folder, not in the CoffeeScript project folder

* Rewrite `getSourceMap` to never read a file from disk, and therefore not throw IO-related exceptions; source maps are either retrieved from memory, or the related source code is retrieved from memory to generate a new source map. Fixes #3890 the proper way.

* Add test to verify that stack traces reference the correct line number. Closes #4418.

* Get the parser working in the browser compiler again; rather than detecting a CommonJS environment generally, just check for `fs` before trying to use it

* Follow Node’s standard of 4-space indentation of stack trace data

* Better .gitignore

* Fix caching of compiled code and source maps; add more tests to verify correct line numbers in stack traces

* Better fallback value for the parser source

* Fix the stack traces and tests when running in a browser

* Update the browser compiler so that @murrayju doesn’t have any extra work to do to test this branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants