Add simple Module System for "include" #1076
Conversation
| @@ -1,5 +1,5 @@ | |||
| include("engines.wine.quick_script.steam_script"); | |||
| include("utils.functions.filesystem.files"); | |||
| const {ls, mkdir, fileExists, cat, cp, getFileSize, fileName, lns, remove, touch, writeToFile, createTempFile, createTempDir, chmod, Checksum} = include("utils.functions.filesystem.files"); | |||
There was a problem hiding this comment.
This allows us to only "include" the functions we actually require.
Currently I include all defined functionality for simplicity reasons but we can change that later on.
There was a problem hiding this comment.
For me, it's not obvious why anybody should know that functions are exported in this order. Also: what happens if you add a function in the middle?
There was a problem hiding this comment.
The order is not important. You can add the "included" functions in any order you want to
There was a problem hiding this comment.
Don't you have to change all includes then?
There was a problem hiding this comment.
What do you mean? What I'm doing here is simple destructuring (see also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment).
So according to my understanding it is ok to use
const {x, y} = include("a");in script "X" and
const {y, z} = include("a");in script "Y"
There was a problem hiding this comment.
From my understanding, x is the first exported function, y the second etc. So if you add a new function between x and y to the module "a", y will suddenly be the new function.
There was a problem hiding this comment.
I think you are confusing this with array destructuring. What I'm doing here is object destructuring. In object destructuring the keys are matched not the indices.
You can test this quite easy yourself in node. Try executing the following code in node:
const x = {a: "a", b: "b", c: "c"};
// destructure "x"
const {c, a} = x;
// print c and a
console.log(a);
console.log(c);The result will be:
a
c
| } | ||
| }; | ||
|
|
||
| module.ls = ls; |
There was a problem hiding this comment.
Alternatively we could export the functions via:
module.ls = function ls(...) {...};I didn't do this here because some functions depend on each other which is problematic because if they are defined using module.x = function x(...) {...}; the method also need to be accessed as module.x from inside the same script.
|
Nice, Codacy helps us now finding unused/unrequired includes! |
|
@qparis we need your input here before we take any decision. |
| @@ -1,5 +1,5 @@ | |||
| include("engines.wine.quick_script.local_installer_script"); | |||
| include("engines.wine.engine.object"); | |||
| const Wine = include("engines.wine.engine.object"); | |||
There was a problem hiding this comment.
I think we need to restructure the engines.wine.engine.object script. The reason being that the script not only exports the engine Wine but also the corresponding version variables like LATEST_STAGING_VERSION. This becomes a problem when accessing an engine from phoenicis (i.e. the Java side), because you need to know the concrete name of the exported engine if you export multiple values at once.
I see two solutions for this:
- we move all exported variables that are not the engine to a new script. This allows us to export the engine using a default export
- we use a fixed name for all engine exports (i.e.
Engine). This solution will lead to a harder to understand code because every script, independent of the corresponding engine type, will useEngineinstead of the concrete engine name, i.e.Wine
There was a problem hiding this comment.
Should the variables even be global variables? Logically, the belong to the Wine "class".
There was a problem hiding this comment.
Good question. What we could do is change the variables to be accessible through static methods of class Wine:
class Wine {
static LATEST_STAGING_VERSION() {
return "version";
}
}This would then require us to use the following to access the most current version:
const Wine = include(...);
Wine.LATEST_STAGING_VERSION()Sadly it is not yet possible to define static fields or accessor methods.
The benefit of returning the variables as a first class member of an object is that we are able to replace them with accessor methods that automatically fetch the most current version and return it:
Object.defineProperty(module, "LATEST_STAGING_VERSION", {
get: function() { return version; }
});This should then make it possible to use:
const {LATEST_STAGING_VERSION} = include(...);
There was a problem hiding this comment.
Can LATEST_STAGING_VERSION be a string module export?
There was a problem hiding this comment.
What do you mean by a string module export?
For now I thought to move LATEST_STAGING_VERSION to a new module. This new module would then look like the following:
/* exported LATEST_STABLE_VERSION */
module.LATEST_STABLE_VERSION = "4.0.1";
/* exported LATEST_DEVELOPMENT_VERSION */
module.LATEST_DEVELOPMENT_VERSION = "4.11";
/* exported LATEST_STAGING_VERSION */
module.LATEST_STAGING_VERSION = "4.11";
/* exported LATEST_DOS_SUPPORT_VERSION */
module.LATEST_DOS_SUPPORT_VERSION = "4.0";Later on we could then think about replacing the hard-coded version strings with more dynamic functions that automatically fetch the corresponding string. To retain the field access syntax for the version strings this would require the usage of
Object.defineProperty(module, "LATEST_STAGING_VERSION", {
get: function() { return version; }
});There was a problem hiding this comment.
Yep, this sounds to be the good approach
|
Overall, I am 100% in phase with this approach. I think it is more reliable than what we used to do and more up to the standards. |
|
The only thing I'm a bit worried about: Or to put it differently: |
|
We could use transpilation in the worst case scenario |
To ensure that other people know how the module system should behave and be used I think we should add a short description to the documentation. I think the PR description in PhoenicisOrg/phoenicis#2050 makes a good first version of this explanation.
I don't think so.
To what should we transpile the code? |
|
ES6 (scripts) to ES5 modules |
|
It seems like we also need to move: Any ideas where to move the variable? I think we use either my newly added versions script: scripts/Engines/Wine/Engine/Versions/script.js Lines 1 to 8 in 5c74ced or we add another script for wine engine constants/variables? |
|
A short status update: I'm not sure yet whether I can fix all such occurrences, because they are really hard to detect without running all scripts. A possible solution here is to cleanup the Lines 41 to 89 in ecd735e I'm not sure whether they are still required with explicit exports. |
|
We should not put the |
- add module to the globals section in the .eslintrc file
- fix two bugs
|
I have good news! I've finished updating the scripts to work with the new module system! In addition I've added some missing includes that eslint mentioned after I removed the corresponding globals from the .eslintrc file. @plata @qparis @ImperatorS79 @Zemogiter can you please test if this PR works for you and whether you have any issues with it? If everything works as intended I think we're ready to have this merged (at least from my side...) |
|
I could but I'm not sure about the POL5 side of this PR because it says it's 2 commits behind master branch. Does it matter? |
|
No the two commits are only dependency updates, but I'll merge the master on the Phoenicis side in a moment. |
|
Just launched your branch of POL5 and currently have only 2 repos on the list - local copy of your script branch and classpath. Wine version tab aka engines tab loads fine, but when right after I clicked install button on subnautica script I got this error: And got this in the terminal log: |
|
Just as reference, the main topics which should be tested are:
|
|
@Zemogiter can you test another installer script? Just to have a comparison? |
|
I've tried Mount&Blade steam script but after I enter the verification code it stays for some time in system monitor then the steam.exe process disappears. But I had this problem on master branches as well. |
|
For Mount&Blade can you share the content of the wine.log file in the container folder? |
|
When installing The german text means: |
|
wine.log: The text in polish means |
|
This looks unrelated to this PR to me. Could this be a problem with the newest Wine version build @plata @qparis @ImperatorS79? |
|
Tested Starcraft 2 script and while the installation works upon launching I get the shortcut error: |
|
@Zemogiter I just fixed the shortcut bug. The reason were missing |
|
The |
|
|
|
@Zemogiter but the include is there, see: |
|
Yes but when I copied the plugin from my PR I accidentaly removed it. |
|
So the scripts are working except the problems shown in the |
|
Yes. Engine tab works and so is the engine tools. |
Added an include in order to be compatible with PhoenicisOrg#1076
- change all scripts to use the new module system - move wine version strings to new script - add missing explicit includes - move WINE_PREFIX_DIR to new constants script - remove now unneeded globals from .eslintrc - remove unused includes from scripts - update documentation pages
This PR changes a few scripts to support the new module system for
include.This PR functions as a preview for discussion purposes.
This PR requires PhoenicisOrg/phoenicis#2050