Re-throw compilation errors with additional information #115
Conversation
index.js
Outdated
| } catch(e) { | ||
| // Create a new error, augment it with location and type information | ||
| // and re-throw. | ||
| let error = new Error(e); |
There was a problem hiding this comment.
Seems slightly odd. Why don't we just add the props we need/want to the existing error? I think as it stands we loose the original stack info?
Also, I believe that broccoli-persistent-filter also wraps in a try / catch so that it can add the file name info...
There was a problem hiding this comment.
I don't think broccoli-persistent-filter wraps in try-catch but maybe I'm wrong? looking here.
as far as why not attaching properties to existing error -- my thinking was that we're going to use one BuildError class to communicate exceptions and would ideally want to just throw new BuildError() w/o add-on developers knowing too much about which properties need to be attached. Having said that, in this particular case, we don't necessarily need to create a new Error object and can re-throw after attaching info we need.
There was a problem hiding this comment.
I don't think broccoli-persistent-filter wraps in try-catch but maybe I'm wrong?
https://github.com/stefanpenner/broccoli-persistent-filter/blob/master/index.js#L283-L288
index.js
Outdated
| } catch(error) { | ||
| // augment with location and type information and re-throw. | ||
| error.type = 'Template Compiler Error'; | ||
| error.location = error.location.start; |
There was a problem hiding this comment.
Error might not be an object and even if it is it may not have location. This catch clause will likely need to be slightly more resilient
There was a problem hiding this comment.
oh? error here comes from ember-template-compiler/broccoli-persistent-filter and I thought it always is going to be an object. As far as location property goes, it should be there because glimmerjs/glimmer-vm#540.
having said that, I'm happy to make it more resilient
There was a problem hiding this comment.
@twokul ya, I would prefer if this was made more resilient as bit-rot or unexpected errors can always change what catch ends up catching.
Its literally the worst to have a catch handler throw when a different bug happened. So if we can beef up this catch handler, it will help mitigate future sadness.
| @@ -7,6 +7,20 @@ const crypto = require('crypto'); | |||
| const stringify = require('json-stable-stringify'); | |||
| const stripBom = require('strip-bom'); | |||
|
|
|||
| function rethrowBuildError(error) { | |||
| if (!error) { throw new Error('Unknown Error'); } | |||
There was a problem hiding this comment.
i believe we can still share realtivePath in some useful way if we throw here. RIght?
There was a problem hiding this comment.
Yes, but that is handled a layer down (in broccoli-persistent-filter itself)
|
released as v2.0.2 🎉 |
Depends on glimmerjs/glimmer-vm#540.
Once we agree on
BuildErrorinterface, we can replace the code in acatchblock to: