Generalize existing Node.js development-time version check #25414
Conversation
…n file that also checks for browser versions.
There was a problem hiding this comment.
Nice!
I was thinking of doing the same thing myself.
I was also thinking that the code for setting ENVIRONMENT_IS_NODE etc could be shared, but that could be followup. I guess we could add that code to this file and call it runtime_environemnt_detect.js or something like that.
| * SPDX-License-Identifier: MIT | ||
| */ | ||
|
|
||
| #include "minimum_runtime_check.js" |
There was a problem hiding this comment.
Maybe just add this to runtime_common.js which is already included in both runtime modes?
There was a problem hiding this comment.
The reason I made a separate file is that this code would be run as the very first thing in the generated .js.
If we put the code somewhere in the middle, we risk already attempting to execute some code that might require a new feature (e.g. ES6 Modules or some language syntax keyword, etc)
src/minimum_runtime_check.js
Outdated
| var currentSafariVersion = typeof navigator !== 'undefined' && navigator?.userAgent?.includes("Safari/") && navigator.userAgent.match(/Version\/(\d+\.?\d*\.?\d*)/) ? humanReadableVersionToPacked(navigator.userAgent.match(/Version\/(\d+\.?\d*\.?\d*)/)[1]) : TARGET_NOT_SUPPORTED; | ||
| if (currentSafariVersion < {{{ MIN_SAFARI_VERSION }}}) throw new Error(`This emscripten-generated code requires Safari v${ packedVersionToHumanReadable({{{ MIN_SAFARI_VERSION }}}) } (detected v${currentSafariVersion})`); | ||
|
|
||
| var currentFirefoxVersion = typeof navigator !== 'undefined' && navigator?.userAgent?.match(/Firefox\/(\d+(?:\.\d+)?)/) ? parseFloat(navigator.userAgent.match(/Firefox\/(\d+(?:\.\d+)?)/)[1]) : TARGET_NOT_SUPPORTED; |
There was a problem hiding this comment.
That would require globalThis, which might not exist in an old browser. This is an ASSERTIONS check anyways, so code size is not that critical.
There was a problem hiding this comment.
Oh I see, we know the globalThis exists on supported browser, but here we are trying to explicitly detect unsupported ones. That makes sense.
I guess you should do check for typeof globalThis first before the other checks? lgtm either way though
src/minimum_runtime_check.js
Outdated
| if (currentFirefoxVersion < {{{ MIN_FIREFOX_VERSION }}}) throw new Error(`This emscripten-generated code requires Firefox v{{{ MIN_FIREFOX_VERSION }}} (detected v${currentFirefoxVersion})`); | ||
|
|
||
| var currentChromeVersion = typeof navigator !== 'undefined' && navigator?.userAgent?.match(/Chrome\/(\d+(?:\.\d+)?)/) ? parseFloat(navigator.userAgent.match(/Chrome\/(\d+(?:\.\d+)?)/)[1]) : TARGET_NOT_SUPPORTED; | ||
| if (currentChromeVersion < {{{ MIN_CHROME_VERSION }}}) throw new Error(`This emscripten-generated code requires Chrome v{{{ MIN_CHROME_VERSION }}} (detected v${currentChromeVersion})`); |
There was a problem hiding this comment.
Maybe wrap this whole file in an IFFE to avoid creating all these global vars?
There was a problem hiding this comment.
Now that this is in an IFFY (so these are just local vars) could we shorten all the currentChromeVersion to just chromeVersion for readability?
| } | ||
|
|
||
| var currentFirefoxVersion = typeof navigator !== 'undefined' && navigator?.userAgent?.match(/Firefox\/(\d+(?:\.\d+)?)/) ? parseFloat(navigator.userAgent.match(/Firefox\/(\d+(?:\.\d+)?)/)[1]) : TARGET_NOT_SUPPORTED; | ||
| if (currentFirefoxVersion < TARGET_NOT_SUPPORTED && 79 == TARGET_NOT_SUPPORTED) { |
There was a problem hiding this comment.
79 == TARGET_NOT_SUPPORTED looks like typo.. is it not?
There was a problem hiding this comment.
That was just runtime test on what could have been a compile time check. Optimized it to take place at compile time.
…vironments # Conflicts: # test/codesize/test_codesize_hello_O0.json # test/codesize/test_codesize_minimal_O0.json # test/codesize/test_unoptimized_code_size.json
…nd require users to pass 'web' or 'node' to disambiguate. Fixes emscripten-core#25414 (review).
…25514) Remove the 'ENVIRONMENT=worker implies ENVIRONMENT=web' assumption. Fixes #25414 (review). Add test to verify the expectation mentioned in that comment.
Generalize existing Node.js development-time version check into common file that also checks for browser versions.