dojo/loader

Update use of runInThisContext

jason0x43 opened this issue · 8 comments

jason0x43 commented

Since 0.12.0 (0.11.7, really), vm.runInThisContext has used an options object for its second argument. The loader should probably use that format, which would also allow it to set the displayErrors flag to improve syntax error output.

tomdye commented

According to the linked spec @jason0x43 , the flag is true by default:

displayErrors: whether or not to print any errors to stderr, with the line of code that caused them highlighted, before throwing an exception. Will capture both syntax errors from compiling code and runtime errors thrown by executing the compiled code. Defaults to true.

tomdye commented

perhaps the output is being trapped somehow or needs to use the reportModuleLoadError functionality added previously to the loader.

https://github.com/dojo/loader/blob/master/src/loader.ts#L900

jason0x43 commented

Good point. Need a test case...

tomdye commented

@jason0x43 I've done some playing around with this and even with the object and a specified fileName attribute etc... it doesn't improve the output due to the default displayErrors value.
Basically, whatever you set the value to, the intern output is always the same and does not include the filename or the actual error location, I think intern may be swallowing this up somewhere?

tomdye commented

I'll change to the following:

vm.runInThisContext(data, {
   'fileName': url.
   'displayErrors': true
});

doesn't achieve anything other than avoiding future accusations though :-)

tomdye commented

PR Raised, issue is still with intern I believe as it is truncating the error output somewhere.

kitsonk commented

This appears to cause issues with Istanbul, where it doesn't recognise the file and therefore doesn't instrument it. We are going to have to revert this (which I will do in a moment)