Rewrite the Script Engine API #2107

madoar · 2019-10-12T12:44:17Z

This PR is still work in progress and at the current time only meant as a preview for discussion purposes!

madoar

In the following I've added some comments about important parts of the PR. Feel free to take a look at the proposed changes so that we can improve the API!

madoar · 2019-10-12T12:46:19Z

...scripts/src/main/java/org/phoenicis/scripts/engine/implementation/PhoenicisScriptEngine.java

+/**
+ * A script engine used by Phoenicis
+ *
+ * @param <R> The internal script result type
+ */
+public interface PhoenicisScriptEngine<R> {
+    /**
+     * Sets a global value for the given key (variable) in the script context
+     *
+     * @param key   The variable name in the script context
+     * @param value The corresponding value
+     */
+    void put(String key, Object value);
+
+    /**
+     * Evaluates the given script and returns its result
+     *
+     * @param script The script
+     * @return The result of the evaluated script
+     */
+    R evaluate(String script);
 }


A script engine now provides a blocking method evaluate which returns the result of the evaluated script in a blocking manner. The result is of a generic type which depends on the concrete script interpreter used underneath (e.g. Value for Graal)

madoar · 2019-10-12T12:47:59Z

...cis-scripts/src/main/java/org/phoenicis/scripts/engine/implementation/TypedScriptEngine.java

+/**
+ * A typed script engine used by Phoenicis
+ *
+ * @param <T> The result type of the script
+ */
+public interface TypedScriptEngine<T> {
+    /**
+     * Evaluates the script with the given id and returns its result cast to the given type
+     *
+     * @param scriptId The id of the evaluated script
+     * @return The cast result of the evaluated script
+     */
+    T evaluate(String scriptId);
+}


This class functions as an intermediate class for direct usage. It returns the result of an evaluated script (this time given by the script id) as an application type (e.g. EngineSetting, Installer etc.)

madoar · 2019-10-12T12:49:58Z

...ipts/src/main/java/org/phoenicis/scripts/engine/implementation/TypedScriptEngineFactory.java

+/**
+ * A factory to create new {@link TypedScriptEngine} instances
+ */
+public interface TypedScriptEngineFactory {
+    /**
+     * Creates a new {@link TypedScriptEngine} instance for the given `type` input
+     *
+     * @param type The result type {@link Class} for the created script engine
+     * @param <T>  The result type
+     * @return The created script engine
+     */
+    <T> TypedScriptEngine<T> createScriptEngine(final Class<T> type);
+}


This class is a factory for TypedScriptEngine instances. At the end I plan to have a single implementation of the factory class which is able to return TypedScriptEngine for all application classes, but because the application classes are currently split into multiple modules we currently need to provide multiple TypedScriptEngineFactory implementations, one for each application class.

What do you mean by "application class"?

By application class/type I mean interfaces like Installer, Engine, EngineSetting, Verb etc.

madoar · 2019-10-12T12:52:03Z

...scripts/src/main/java/org/phoenicis/scripts/engine/implementation/PhoenicisScriptEngine.java

-
-    void put(String name, Object object, Consumer<Exception> errorCallback);
-
-    void addErrorHandler(Consumer<Exception> errorHandler);


Because I'm removing the error handlers contained in the script engine we need to provide them nearer to where they are needed in specific try/catch blocks. I think this is a better approach because it is currently very hard to know what error handlers are called for a script because they are often specified in an entirely different code region.

madoar · 2019-10-12T12:53:11Z

...nicis-scripts/src/main/java/org/phoenicis/scripts/engine/implementation/GraalEngineType.java

+public class GraalEngineType implements ScriptEngineType<Value> {
+    @Override
+    public PhoenicisScriptEngine<Value> createScriptEngine() {
+        return new PolyglotScriptEngine("js",
+                Map.of("js.nashorn-compat", "true",
+                        "js.experimental-foreign-object-prototype", "true"));
+    }
+
+    @Override
+    public String toString() {
+        return "graal.js";
+    }
+}


Here the graal.js specific implementation of the ScriptEngineType

madoar · 2019-10-12T12:53:57Z

phoenicis-engines/src/main/java/org/phoenicis/engines/EngineSettingsManager.java

     */
-    public EngineSettingsManager(PhoenicisScriptEngineFactory phoenicisScriptEngineFactory,
-            ExecutorService executorService) {
+    public EngineSettingsManager(TypedScriptEngineFactory typedScriptEngineFactory,


This class shows an example how I intend to use the script engine in the future.

Does it mean that we cannot access e.g. applications and engines via the same script engine?

Yes. We will need to provide a dedicated TypedScriptEngine implementation for every application type (i.e. Installer, Engine, etc)

Understood. So this is also because we want to keep the APIs in the modules and not centralize them in phoenicis-scripts (we discussed this somewhere else).

So this is also because we want to keep the APIs in the modules and not centralize them in phoenicis-scripts (we discussed this somewhere else).

No the reason why I want to implement a dedicated TypedScriptEngine class for every application type is because it divides the functionality better. It follows the approach divide and conquer in that we implement independent behavior in different modules/classes. For example what have the processing of Installer script and Verb script in common except that they are both executed with graaljs and use the module system? Therefore we divide the corresponding functionality in different classes.

I still plan to move all application types (and ideally also the TypeScriptEngine implementations) to the phoenicis-scripts module. I didn't do this yet because I think the main changes of this PR are easier comprehensible if I leave as much of the related refactoring as possible for later.

I still plan to move all application types

I thought @qparis doesn't like this?

madoar · 2019-10-12T12:54:31Z

phoenicis-engines/src/main/java/org/phoenicis/engines/EngineSettingScriptEngine.java

+public class EngineSettingScriptEngine implements TypedScriptEngine<EngineSetting> {
+    private final PhoenicisScriptEngine<Value> scriptEngine;
+
+    public EngineSettingScriptEngine(PhoenicisScriptEngine<Value> scriptEngine) {
+        super();
+
+        this.scriptEngine = scriptEngine;
+    }
+
+    @Override
+    public EngineSetting evaluate(String scriptId) {
+        final String script = String.format("include(\"%s\")", scriptId);
+
+        return scriptEngine.evaluate(script).newInstance().as(EngineSetting.class);
+    }
+}


Here the TypedScriptEngine for the EngineSetting application class

plata · 2019-10-13T07:32:58Z

Is this something you want to do before alpha-3?

madoar · 2019-10-13T09:42:07Z

Ideally yes, but I'm not sure about our schedule for alpha-3.

plata · 2019-10-14T17:46:05Z

I have no clue regarding the schedule. There are some packaging issue which cannot fix.

madoar · 2019-10-14T20:06:30Z

I think we will need to wait until @qparis has more time before we can fix those.

I'm also currently a bit limited on time I can invest in this PR. Therefore feel free to give the PR an in-depth look so that we have a stable and durable solution when we finally merge the PR!

- begin rewriting the script engine API

f5104ad

madoar commented Oct 12, 2019

View reviewed changes

madoar requested review from plata and qparis October 12, 2019 12:55

madoar mentioned this pull request Dec 26, 2019

Git scripts repository update fails #2130

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite the Script Engine API #2107

Rewrite the Script Engine API #2107

madoar commented Oct 12, 2019

madoar left a comment

madoar Oct 12, 2019

madoar Oct 12, 2019

madoar Oct 12, 2019

plata Oct 13, 2019

madoar Oct 13, 2019

madoar Oct 12, 2019

madoar Oct 12, 2019

madoar Oct 12, 2019

plata Oct 13, 2019

madoar Oct 13, 2019

plata Oct 14, 2019

madoar Oct 14, 2019

plata Oct 15, 2019

madoar Oct 12, 2019

plata commented Oct 13, 2019

madoar commented Oct 13, 2019

plata commented Oct 14, 2019

madoar commented Oct 14, 2019


		void put(String name, Object object, Consumer<Exception> errorCallback);

		void addErrorHandler(Consumer<Exception> errorHandler);

Rewrite the Script Engine API #2107

Are you sure you want to change the base?

Rewrite the Script Engine API #2107

Conversation

madoar commented Oct 12, 2019

madoar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plata commented Oct 13, 2019

madoar commented Oct 13, 2019

plata commented Oct 14, 2019

madoar commented Oct 14, 2019