{"action":"created","issue":{"url":"https://api.github.com/repos/magnars/optimus/issues/52","repository_url":"https://api.github.com/repos/magnars/optimus","labels_url":"https://api.github.com/repos/magnars/optimus/issues/52/labels{/name}","comments_url":"https://api.github.com/repos/magnars/optimus/issues/52/comments","events_url":"https://api.github.com/repos/magnars/optimus/issues/52/events","html_url":"https://github.com/magnars/optimus/pull/52","id":152250182,"number":52,"title":"Use JSR-223 API to select a JS engine","user":{"login":"arbscht","id":42454,"avatar_url":"https://avatars.githubusercontent.com/u/42454?v=3","gravatar_id":"","url":"https://api.github.com/users/arbscht","html_url":"https://github.com/arbscht","followers_url":"https://api.github.com/users/arbscht/followers","following_url":"https://api.github.com/users/arbscht/following{/other_user}","gists_url":"https://api.github.com/users/arbscht/gists{/gist_id}","starred_url":"https://api.github.com/users/arbscht/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/arbscht/subscriptions","organizations_url":"https://api.github.com/users/arbscht/orgs","repos_url":"https://api.github.com/users/arbscht/repos","events_url":"https://api.github.com/users/arbscht/events{/privacy}","received_events_url":"https://api.github.com/users/arbscht/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":8,"created_at":"2016-05-01T16:23:44Z","updated_at":"2016-05-02T15:52:16Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/magnars/optimus/pulls/52","html_url":"https://github.com/magnars/optimus/pull/52","diff_url":"https://github.com/magnars/optimus/pull/52.diff","patch_url":"https://github.com/magnars/optimus/pull/52.patch"},"body":"Changes:\r\n\r\n* Stop depending on clj-v8 directly; use it via clj-jsr223-v8 and `javax.script.*\".\r\n* New ns `optimus.js` talks to `javax.script.*` API to get/control script engines.\r\n* `optimus.optimizations.minify` uses `optimus.js` to create contexts and evaluate scripts.\r\n* Engines can be chosen by configuring the `:optimus-js-engine` setting via environ.\r\n  * This can be set or overridden in `profiles.clj`, shell environment, or anywhere else as [documented](https://github.com/weavejester/environ#usage).\r\n  * e.g. in the Optimus project, `OPTIMUS_JS_ENGINE=nashorn lein midje` will run tests using Nashorn (assuming JDK8).\r\n  * Engine names are defined by the engines. Nashorn goes by \"nashorn\"; V8 via clj-jsr223-v8 goes by \"clj-v8\" (default selection). \"rhino\" or \"dynjs\" should also work if those libraries are installed.\r\n\r\nTesting:\r\n\r\n* Some engine control tests added under `optimus.js-test`.\r\n* Using `openjdk version \"1.8.0_77-Debian\"`, `lein midje` reports \"All checks (1908) succeeded.\" for either \"clj-v8\" or \"nashorn\" as `:optimus-js-engine`.\r\n* Needs more real-world testing…\r\n\r\nSome other aspects to review:\r\n\r\n* \"context\" is a confusingly overloaded term, so I started using \"engine\" for one sense in the code. Maybe there a better and more consistent terminology.\r\n* Is environ a good way to detect engine choice, considering how Optimus is otherwise configured?\r\n* There is no fallback configuration engine, only a default. (I don't think there should be a fallback if the user names an invalid engine…)\r\n* clj-jsr223-v8 changes evaluation semantics compared to directly using clj-v8, and I especially don't know how much it affects performance.\r\n  * Input scripts are evaluated using JS `eval(…)`, not directly as before.\r\n  * Returned values are unmarshalled JSON values (via `JSON.stringify` in JS and `clojure.data.json/read-str in Clojure`).\r\n* `cleanup-engine` is optional if relying on `finalize()` is acceptable (but I've left it in anyway for now)."},"comment":{"url":"https://api.github.com/repos/magnars/optimus/issues/comments/216274570","html_url":"https://github.com/magnars/optimus/pull/52#issuecomment-216274570","issue_url":"https://api.github.com/repos/magnars/optimus/issues/52","id":216274570,"user":{"login":"arbscht","id":42454,"avatar_url":"https://avatars.githubusercontent.com/u/42454?v=3","gravatar_id":"","url":"https://api.github.com/users/arbscht","html_url":"https://github.com/arbscht","followers_url":"https://api.github.com/users/arbscht/followers","following_url":"https://api.github.com/users/arbscht/following{/other_user}","gists_url":"https://api.github.com/users/arbscht/gists{/gist_id}","starred_url":"https://api.github.com/users/arbscht/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/arbscht/subscriptions","organizations_url":"https://api.github.com/users/arbscht/orgs","repos_url":"https://api.github.com/users/arbscht/repos","events_url":"https://api.github.com/users/arbscht/events{/privacy}","received_events_url":"https://api.github.com/users/arbscht/received_events","type":"User","site_admin":false},"created_at":"2016-05-02T15:52:16Z","updated_at":"2016-05-02T15:52:16Z","body":"Well, what do you know. Nashorn implements `Array.splice` [differently](https://bugs.openjdk.java.net/browse/JDK-8023252) (correctly) compared to pretty much everyone else, including v8 and spidermonkey. CleanCSS assumes that splice behaves the typical but non-standard way deep in a code path under advanced optimizations.\r\n\r\nMeanwhile, Avatar JS takes the approach of monkey-patching `Array.prototype.splice` for compatibility. I've updated ns optimus.js to do something similar with a generic `init-engine` multimethod.\r\n\r\nI think there are not likely to be very many of these issues, so I'd continue to have confidence in Nashorn.\r\n\r\nThe suspect splice call is at `clean-css.js:10467`:\r\n```js\r\n  while (tokenIdx >= 0) {\r\n     if ((tokenIdx === 0 || (optimizedBody.tokenized[propertyIdx] && bodiesAsList[tokenIdx].indexOf(optimizedBody.tokenized[propertyIdx].value) > -1)) && propertyIdx > -1) {\r\n      propertyIdx--;\r\n      continue;\r\n    }\r\n\r\n    var newBody = {\r\n      list: optimizedBody.list.splice(propertyIdx + 1),\r\n      tokenized: optimizedBody.tokenized.splice(propertyIdx + 1)\r\n    };\r\n    options.callback(tokens[processedTokens[tokenIdx]], newBody, processedCount, tokenIdx);\r\n\r\n    tokenIdx--;\r\n  }\r\n```\r\n\r\nSo I added a new `init-engine` hook which executes this in every Nashorn engine:\r\n\r\n```js\r\n(function(){\r\n  var nashorn_splice = Array.prototype.splice;\r\n  Array.prototype.splice = function() {\r\n  if (arguments.length === 1) {\r\n   return nashorn_splice.call(this, arguments[0], this.length);\r\n  } else {\r\n   return nashorn_splice.apply(this, arguments);\r\n }}})();\r\n```"}}