-
Notifications
You must be signed in to change notification settings - Fork 23
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
Feat environment variables #192
Conversation
3824642
to
7f51d6b
Compare
This PR should be ready for review. This PR should not require the changes in cheerp-libs |
stream << "var " << EnvironName << "=[],"; | ||
stream << ArgvName << "=[];" << NewLine; |
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'm not a 100% sure about this, as currently this is always emitted at the start of the file
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.
also, does it work when using a closure module? you probably have to use window/this/globalThis in that case to force the variables to be visible outside.
Also, maybe we could only have one variable, with two fields argv and env? but I don't have a good default name for it.
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.
Based on a quick test, it seems like it also works for closure modules
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.
it does not work with a closure module or with no modules: you are defining the variables here, but really they should be defined from the outside, in the global scope, and you just use them here (checking that they exist).
I wrote a quick test, and if I just delete these lines altogether, it works. Otherwise, the content is always empty.
a4d7de4
to
ed3b767
Compare
The CheerpJ PR should be merged before this one |
I didn't add the new builtins to the header as I didn't want to expose this to the average user, but they are now declared with the correct type in cheer-libs: |
Also merge this PR last (i.e. cheerp-libs and cheerp-utils first) |
Emit global variables and their necessay initialization that can contain program arguments/environment variables. These global variables are string arrays. Also add two builtins for internal use to get these globals: const client::TArray<client::String>* __builtin_cheerp_environ(void); const client::TArray<client::String>* __builtin_cheerp_argv(void);
This is no longer needed as environment variables are now natively supported by Cheerp.
Please note that this is not yet finished and just for early review