fix: modify script evaluation for IE11
#41
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.
Using Todo in code is bad practice. How we can solve this problem, what you think?
There was a problem hiding this comment.
@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.
@markus-wi i think console.log is good solution.
There was a problem hiding this comment.
@evilebottnawi ok, I've added the error logging and updated my pull request
alexander-akait
assigned michael-ciniawsky and joshwiens and unassigned michael-ciniawsky and joshwiens
alexander-akait
requested review from
alexander-akait,
michael-ciniawsky and
joshwiens
addScript.js
Outdated
| } else if (typeof execScript !== "undefined") { | ||
| execScript(src); | ||
| } else { | ||
| console.log("no eval function available"); |
There was a problem hiding this comment.
console.error('[Script Loader] EvalError: No eval function available')There was a problem hiding this comment.
@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.
So the try/catch is solely for errors within eval ?
There was a problem hiding this comment.
@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.
console.error('[Script Loader] ', err.message)
Is the |
michael-ciniawsky
changed the title
script evaluation for IE11
As far as I know, execScript is needed for IE < 11 compatibility |
michael-ciniawsky
added
pr_action: Review
pr_action: Merge ready
status: Approved
and removed
pr_action: Rework
pr_action: Review
labels
|
👍 I just tested this myself and it works great. Solves the problem beautifully. |
Pull Request (Notable Changes)
I've moved up the check for
evalbecause IE11 still has theexecScriptfunction, 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