#1099@minor: Add support for currentScript. #1100
Conversation
ckhampus
force-pushed
the
task/1099-add-document-current-script
branch
from
1c0174a to
cd32a53
Compare
| WindowErrorUtility.captureError(this.ownerDocument.defaultView, () => | ||
| this.ownerDocument.defaultView.eval(textContent) | ||
| ); | ||
| try { |
There was a problem hiding this comment.
We should not do a try and catch here. Thats why we have the WindowErrorUtility.captureError() method that only should be used if this.ownerDocument.defaultView.happyDOM.settings.disableErrorCapturing is set to false.
If error capturing is disabled, we want the error to be thrown and catched by the framework using Happy DOM.
There was a problem hiding this comment.
We should also add the this.ownerDocument['_currentScript'] = * logic to HTMLScriptElementUtility when it is evaluating the code.
There was a problem hiding this comment.
Thanks for the feedback!
We should not do a try and catch here. Thats why we have the
WindowErrorUtility.captureError()method that only should be used ifthis.ownerDocument.defaultView.happyDOM.settings.disableErrorCapturingis set to false.If error capturing is disabled, we want the error to be thrown and catched by the framework using Happy DOM.
This try does not actually catch any errors because there isn't a catch-block, and it just has a finally-block so we can set this.ownerDocument['_currentScript'] to null. So any errors should passed through as before.
We should also add the
this.ownerDocument['_currentScript'] = *logic toHTMLScriptElementUtilitywhen it is evaluating the code.
I will fix that! 👍
There was a problem hiding this comment.
The try block slows down execution a lot as it will still create a new context for each try to capture errors.
If we encounter and error when this.ownerDocument.defaultView.happyDOM.settings.disableErrorCapturing is set to true, I think it is expected that the execution stops anyway.
You can see a performance test of try/finally here:
Link to test
There was a problem hiding this comment.
Ahh I see, thanks for the link and info. Didn't know that. I will change it then 🙂
…n loading a script file.
|
I have fixed the last things and merged it now 🙂 |
Fixes #1099