switch to expr-eval for dynamic module

[jywarren]

Fixes #1728 by replacing eval() with expr-eval for mathematical expressions

[gitpod-io]

[harshkhandeparkar]

It's breaking, I think we should do this in 3.6.1.

[jywarren]

Ok no problem, sounds good! Should be pretty simple to fix then. Thanks!!

…

On Sat, Oct 17, 2020, 3:25 PM Harsh Khandeparkar ***@***.***> wrote: It's breaking, I think we should do this in 3.6.1. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1729 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J77PAGTFKEWTHOC4QTSLHVRVANCNFSM4SURIZUA> .

[jywarren]

I'm seeing some MinifyImage issues related to => the ES6 function syntax... odd.

[harshkhandeparkar]

npm install must have bumped some of the deeper dependencies in the lockfile. This is the same reason that is causing the audits to fail...

[jywarren]

aha! also:

      throw new Error('parse error [' + coords.line + ':' + coords.column + ']: Expected ' + (value || type));
      ^
Error: parse error [1:6]: Expected EOF
    at ParserState.expect (/home/travis/build/publiclab/image-sequencer/node_modules/expr-eval/dist/bundle.js:1049:13)
    at Parser.parse (/home/travis/build/publiclab/image-sequencer/node_modules/expr-eval/dist/bundle.js:1772:17)
    at Function.Parser.parse (/home/travis/build/publiclab/image-sequencer/node_modules/expr-eval/dist/bundle.js:1784:25)
    at generator (/home/travis/build/publiclab/image-sequencer/src/modules/Dynamic/Module.js:9:1245)
    at /home/travis/build/publiclab/image-sequencer/src/modules/Dynamic/Module.js:9:1952
    at Array.forEach (<anonymous>)
    at Object.draw (/home/travis/build/publiclab/image-sequencer/src/modules/Dynamic/Module.js:9:1505)
    at drawStep (/home/travis/build/publiclab/image-sequencer/src/Run.js:9:2704)
    at drawSteps (/home/travis/build/publiclab/image-sequencer/src/Run.js:9:3842)
    at Run (/home/travis/build/publiclab/image-sequencer/src/Run.js:9:4953)

[jywarren]

Now the only error seems to be:

> TEST=true istanbul cover tape test/core/*.js test/core/ui/user-interface.js test/core/modules/*.js | tap-spec;

    Failed to parse file: /home/runner/work/image-sequencer/image-sequencer/src/modules/MinifyImage/Module.js
Transformation error; return original code
Error: Line 67: Unexpected token =>

which is an ES6 issue.

Sorry, that's not an issue here, that occurs in main branch too and i made an issue: #1810

OK, so what else in that test run?

  dynamic module works correctly with different options


    ✖ dynamic module works correctly with initial option undefined
    ---------------------------------------------------------------
      operator: equal
      expected: true
      actual:   false
      at: Immediate._onImmediate (/home/runner/work/image-sequencer/image-sequencer/node_modules/looks-same/lib/utils.js:75:17)
      stack: |-
  Error: dynamic module works correctly with initial option undefined
  at Test.assert [as _assert] (/home/runner/work/image-sequencer/image-sequencer/node_modules/tape/lib/test.js:228:54)
  at Test.bound [as _assert] (/home/runner/work/image-sequencer/image-sequencer/node_modules/tape/lib/test.js:80:32)
  at Test.equal (/home/runner/work/image-sequencer/image-sequencer/node_modules/tape/lib/test.js:389:10)
  at Test.bound [as equal] (/home/runner/work/image-sequencer/image-sequencer/node_modules/tape/lib/test.js:80:32)
  at /home/runner/work/image-sequencer/image-sequencer/test/core/templates/options-test.js:40:11
  at /home/runner/work/image-sequencer/image-sequencer/node_modules/looks-same/index.js:183:13
  at Immediate._onImmediate (/home/runner/work/image-sequencer/image-sequencer/node_modules/looks-same/lib/utils.js:75:17)
  at processImmediate (internal/timers.js:461:21)


    ✖ dynamic module works correctly when the option is changed to undefined
    -------------------------------------------------------------------------
      operator: equal
      expected: true
      actual:   false
      at: Immediate._onImmediate (/home/runner/work/image-sequencer/image-sequencer/node_modules/looks-same/lib/utils.js:75:17)
      stack: |-
  Error: dynamic module works correctly when the option is changed to undefined
  at Test.assert [as _assert] (/home/runner/work/image-sequencer/image-sequencer/node_modules/tape/lib/test.js:228:54)
  at Test.bound [as _assert] (/home/runner/work/image-sequencer/image-sequencer/node_modules/tape/lib/test.js:80:32)
  at Test.equal (/home/runner/work/image-sequencer/image-sequencer/node_modules/tape/lib/test.js:389:10)
  at Test.bound [as equal] (/home/runner/work/image-sequencer/image-sequencer/node_modules/tape/lib/test.js:80:32)
  at /home/runner/work/image-sequencer/image-sequencer/test/core/templates/options-test.js:57:13
  at /home/runner/work/image-sequencer/image-sequencer/node_modules/looks-same/index.js:183:13
  at Immediate._onImmediate (/home/runner/work/image-sequencer/image-sequencer/node_modules/looks-same/lib/utils.js:75:17)
  at processImmediate (internal/timers.js:461:21)

Let's see if it passed earlier tests. Maybe our new expr-eval doesn't handle undefineds and we need to screen.

[jywarren]

This looks like it could be a similar issue as @blurry-x-face faced using looks-same, as I can't see any other reason it'd be different. The test that's failing is this, and we haven't changed much before/after:

image-sequencer/test/core/templates/options-test.js

Lines 37 to 41 in 0cbefa3

	looksSame(result, benchmark[0], function(err, res) {
	if (err) console.log(err);
	t.equal(res.equal, true, `${moduleName} module works correctly with initial option ${options[0]}`);
	});

Although perhaps it's that the expr-eval package is generating a function differently. Let me add a test for that.

[jywarren]

OK, got it - this implementation didn't work, the generator function outputs:

> generator('B+B')+''
'function () {\n      return f.apply(expr, arguments);\n    }'

[jywarren]

OK it was struggling with undefineds in let expr = Parser.parse('R = r; G = g; B = b; A = a; ' + expression); so i'll just copy the options rather than try to assign them within the expression

[jywarren]

Nice! @harshkhandeparkar got this to work!!!

[harshkhandeparkar]

Wow! 🎉