-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add ENVIRONMENT to specify which environment the code will run in #6565
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
Conversation
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.
Nice overall
@@ -30,11 +30,13 @@ mergeInto(LibraryManager.library, { | |||
var uuid = null; | |||
|
|||
if (ENVIRONMENT_IS_NODE) { | |||
#if ENVIRONMENT_MAY_BE_NODE |
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.
Is it profitable to move the if (ENVIRONMENT_IS_NODE) {
part inside the #if
? Our expectation is that if environment is set to not-X, then it's not expected to work if it is X, so we shouldn't need to guard against it. (As I understand it, if the environment is node, we have an empty if
body here, so this shouldn't work anyway).
Are we able to optimize out the if
with the JS Optimizer so it doesn't matter either way?
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.
Moving it outside was a little messier - then there is the if-else chain to handle (where to put else
s, that is, if we have if (..) { } else if (..)
then we can't remove the first if until the else, we also need to remove the else).
The JS optimizer is not capable of doing this, however, closure is, so for really-optimized builds it shouldn't matter. Would be nice to find a clean way to make it happen without closure too, at low priority I'd say.
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.
How about:
#if A
if (A) {
//...
} else
#endif // A
#if B
if (B) {
//...
} else
#endif // B
#if C
// ...
#endif // Q
{
//...
}
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.
Good idea, rewritten.
Impressed to see this! Happy with it. Thanks for the quick turnaround. |
By default we still emit code that can run in node, web, shell, etc., but with the option the user can fix only one to be emitted.
This also sets ENVIRONMENT to "web" if the user specifies an HTML output (
emcc [..] -o code.html
). In that case the indication is clear that the code should only run on the web. This is technically a breaking change though, as people may have tried to run the.js
file outside of the web, and that may have worked in some cases. To prevent confusion, whenASSERTIONS
are on we show an error if we hit a problem, and also in debug logging we mention doing HTML output setting the environment that way. The benefit to doing this is that it gets all the benefits of web-only emitting in a natural way for the main use case.For more details on the benefits, see #6542 #1140, but basically
require()
operations that confuse bundlers.