-
Notifications
You must be signed in to change notification settings - Fork 44
Conversation
addScript.js
Outdated
} else if (typeof execScript !== "undefined") { | ||
execScript(src); | ||
} else { | ||
// TODO error: no eval function available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Todo
in code is bad practice. How we can solve this problem, what you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evilebottnawi As I mention in the description of my pull request, I don't know how these errors should be handled. We could just log them ( console.log("no eval function available")
) or throw them ( throw new Error("no eval function available")
), so that they have to be handled outside of the script loader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Markus-WI i think console.log
is good solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evilebottnawi ok, I've added the error logging and updated my pull request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
addScript.js
Outdated
} else if (typeof execScript !== "undefined") { | ||
execScript(src); | ||
} else { | ||
console.log("no eval function available"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.error('[Script Loader] EvalError: No eval function available')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michael-ciniawsky it can be break execution there eval
and execScript
not allowed or forbidden (CSP). Maybe best output error
than break execution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the try/catch
is solely for errors within eval
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michael-ciniawsky I've updated my commit accordingly.
addScript.js
Outdated
console.log("no eval function available"); | ||
} | ||
} catch (error) { | ||
console.log("script evaluation error occurred", error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.error('[Script Loader] ', err.message)
Is the |
script
evaluation for IE11
As far as I know, execScript is needed for IE < 11 compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Markus-WI Thx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
👍 I just tested this myself and it works great. Solves the problem beautifully. |
Pull Request (Notable Changes)
I've moved up the check for
eval
because IE11 still has theexecScript
function, but throws an error when it's called.The added error handling allows us to gracefuly reject scripts with non compatible code (like ES6 syntax). It would be better if we could transpile with babel before evaluating, but I don't know how we could implement that. There's already a separate issue regarding this feature (See #40).
Issues
Fixes #36
Fixes #39