-
Notifications
You must be signed in to change notification settings - Fork 48
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.
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
Add simple Module System for "include" #1076
Changes from 1 commit
b9dde43
b755ff2
5c74ced
47d5c5f
db35147
24ee472
d7c7073
4ea3d74
8ab9130
de4ed69
44c92c7
4c71ff0
6eeac4f
921ed1e
4c87b16
bceb02f
9b0bd37
c79469f
75deed4
4532809
0b1f90b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to restructure the I see two solutions for this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the variables even be global variables? Logically, the belong to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. What we could do is change the variables to be accessible through static methods of class 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can LATEST_STAGING_VERSION be a string module export? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean by /* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, this sounds to be the good approach |
||
include("engines.wine.plugins.csmt"); | ||
include("engines.wine.plugins.windows_version"); | ||
include("engines.wine.verbs.d3dx9"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order is not important. You can add the "included" functions in any order you want to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you have to change all includes then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
in script "X" and
in script "Y"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding,
x
is the first exported function,y
the second etc. So if you add a new function betweenx
andy
to the module"a"
,y
will suddenly be the new function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
The result will be:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then it's fine.