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

Slightly better error reporting for coffeescript. #338

Merged
merged 1 commit into from Sep 16, 2012

Conversation

tmeasday
Copy link
Contributor

Fixes #331

I just copied what the less package does. Not sure why, but it seems to try to compile the file twice.

@glasser
Copy link
Contributor

glasser commented Sep 15, 2012

The "compile file twice" is presumably referring to .coffee files that are in the main app dir, and thus are compiled once for client and once for server. (While the compilation is identical in both cases, one could imagine register_extension handlers that do significantly different things on client and server.)

try {
contents = new Buffer(coffee.compile(contents.toString('utf8'), options));

bundle.add_resource({
Copy link
Contributor

Choose a reason for hiding this comment

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

I like to try to keep the scope of a try/catch as small as possible, so I'd just wrap the "contents = " line in the try/catch, or in fact even just the "coffee.compile" call and move new Buffer to its own line. That way, an exception through during "new Buffer" or "bundle.add_resource" won't confusingly be described as a Coffeescript compiler error. I realize that you are using the "less" package as inspiration, but that call uses an asynchronous callback, so in fact even though it physically looks similar to what you've written here, the try/catch really is only surrounding the less.render call.

That said, taking a peek at the coffeescript source, it looks like if you add a "filename" key to the "options" object, coffeescript itself will include the filename in all errors.

@tmeasday
Copy link
Contributor Author

@glasser Thanks for looking at this properly. This much simpler change works. So my question now is: should we be catching CS's error and doing bundle.error, or should we let it blow meteor up like it does right now?

@glasser
Copy link
Contributor

glasser commented Sep 15, 2012

Oh, good point. So you probably want something like
try {
var compiled = coffee.compile(...);
} catch (e) {
bundle.error(e.message);
return;
}

Use CS's inbuilt filename reporting, and use `bundle.error` rather than throwing.
Fixes meteor#331
@tmeasday
Copy link
Contributor Author

Does that seem reasonable?

@glasser glasser merged commit 6a4f20a into meteor:devel Sep 16, 2012
StorytellerCZ pushed a commit that referenced this pull request Oct 1, 2021
* Add import/export statements to Secret server code

* Fix MMR require

* Fix excessive use of const 😄
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 this pull request may close these issues.

Problems in CoffeeScript scripts do not include the file name
2 participants