Skip to content

Remove dependency on eval #132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
buu700 wants to merge 2 commits into from
Closed

Remove dependency on eval #132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30f3bb4
remove duplicate dev dependency
buu700 Apr 27, 2017
e7a455e
remove dependency on eval
buu700 Apr 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
"*.min.js",
"*.min.js.map"
],
"dependencies": {
"math-expression-evaluator": "^1.2.17"
},
"devDependencies": {
"mocha": "1.21.4",
"chai": "^1.9.1",
Expand All @@ -36,7 +39,6 @@
"karma-sauce-launcher": "~0.2.3",
"grunt-checkrepo": "~0.1.0",
"grunt-saucelabs": "~4.0.2",
"grunt-checkrepo": "~0.1.0",
"grunt-git-status": "~1.0.0",
"grunt-template": "~0.2.3",
"source-map": "~0.1.40"
Expand Down
4 changes: 3 additions & 1 deletion src/dimension-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

(function(scope, testing) {

var mexp = require('math-expression-evaluator');
Copy link
Contributor

@dstoc dstoc May 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't nodejs code, require wont work here.

Copy link
Author

@buu700 buu700 May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


function parseDimension(unitRegExp, string) {
string = string.trim().toLowerCase();

Expand Down Expand Up @@ -54,7 +56,7 @@
return;

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'));
Copy link
Contributor

@dstoc dstoc May 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@buu700 buu700 May 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's 11.7 kb.

Copy link
Contributor

@ewilligers ewilligers Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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.

Copy link
Author

@buu700 buu700 Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, that's great news!

if (!isFinite(result))
return;
matchedUnits[unit] = result;
Expand Down