Remove dependency on eval #132
Conversation
|
Looks like the tests are failing because they aren't configured to work with dependencies/ Tests aside, it seems to be working as expected in my project that uses @angular/animations. |
|
Caught an edge case regression: repeatedly mousing in and out of an element with an @angular/material mdTooltip will at some point throw Edit: disregard this; apparently I just hadn't noticed that |
buu700
force-pushed
the
dev
branch
2 times, most recently
from
35a6750 to
ba5e13a
Compare
|
@buu700 Hi, I am creator of mexp. As far as I know, mexp.eval returns a Number with precision upto 15 digits. May you please tell me in which case it is returning a string? |
|
It looks like it's consistently returning a string in both a node repl and my browser console:
|
|
I have fixed this. Now it is returning numbers in version 1.2.17 |
|
Nice, thanks! I'll update this pr to remove |
|
|
||
| (function(scope, testing) { | ||
|
|
||
| var mexp = require('math-expression-evaluator'); |
There was a problem hiding this comment.
This isn't nodejs code, require wont work here.
There was a problem hiding this comment.
I wasn't sure how you guys would want to handle dependencies, since this would be the first one. Is there a solution that would be preferred over require with a module bundler?
One option that comes to mind is something like typeof require !== 'undefined' ? require('math-expression-evaluator').eval : eval, which would accommodate both users like me who can't use eval for CSP compliance and users who want the size advantage you mentioned of not including another dependency (either by loading web-animations-js with a script tag or by configuring math-expression-evaluator as an undefined external in their module bundler).
|
|
||
| for (var unit in matchedUnits) { | ||
| var result = eval(string.replace(new RegExp('U' + unit, 'g'), '').replace(new RegExp(taggedUnitRegExp, 'g'), '*0')); | ||
| var result = mexp.eval(string.replace(new RegExp('U' + unit, 'g'), '').replace(new RegExp(taggedUnitRegExp, 'g'), '*0')); |
There was a problem hiding this comment.
The man reason eval is used here is to keep the code size small. How large is this dependency? Further, if we're going to replace this, I think we would prefer to write the replacement inline rather than adding dependencies to this library.
There was a problem hiding this comment.
#151 implements calc expression inline and adds 449 bytes to web-animations-next-lite.min.js. No need to add an 11.7 kb dependency.
4 participants
I added a dependency on math-expression-evaluator, which as far as I can tell should handle every expected input that's currently being passed in to eval.