Drop unuseful eval call
#8385
Drop unuseful eval call
#8385
Conversation
If use CSP (without `unsafe-eval`), both `new Function` and `eval` will be forbidden, so `eval` part is just dead code.
The only possibility `eval` part would run was `new Function('return this')()` return a falsy value, but it never possible as the spec (Is there any old engine violate this? Never heard about that.)
This pr also add `new` keyword to make the code a little bit explicit.
|
|
|
For maintainers only:
|
| @@ -7,7 +7,7 @@ g = (function() { | |||
|
|
|||
| try { | |||
| // This works if eval is allowed (see CSP) | |||
| g = g || Function("return this")() || (1, eval)("this"); | |||
| g = g || new Function("return this")(); | |||
There was a problem hiding this comment.
If both are equal, why not using (1,eval)("this") only instead. That's shorter.
There was a problem hiding this comment.
Because "eval is evil" 🤪
A better reason:
We use function() {return this;} before, so new Function("return this") is just same code. Actually new Function("return this") is just like function() {"use non-strict"; return this}. If no CSP, we should simply use new Function, if we have "use non-strict" directive, we should simply use function () { ... }
It's possible to only use eval, if that, we should also change function () {...} to eval and it's shorter as you say.
The real bad part of eval is the magic of (1, eval) --- it use rarely used commas expression to get indirect eval which always executed in global context, and direct vs indirect eval is also a unintelligible thing for many js programmers. In fact, I go to here because someone just ask me what (1, eval) means in webpack code! And I myself ask other guys to recall the direct/indirect eval issue (I taught many programmers about that in ES5 era, but I forgot it now!!!)
So I think we'd better use the much simple new Function way. 😆
|
Have no idea why there is a test fail... incidental timeout? |
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
|
Thanks |
If use CSP (without
unsafe-eval), bothnew Functionandevalwill be forbidden, soevalpart is just dead code.The only possibility
evalpart would run wasnew Function('return this')()return a falsy value, but it never possible as the spec (Is there any old engine violate this? Never heard about that.)This pr also add
newkeyword to make the code a little bit explicit.What kind of change does this PR introduce?
clean unuseful code
Did you add tests for your changes?
no need
Does this PR introduce a breaking change?
no
What needs to be documented once your changes are merged?
no need