Modernize and run eslint on tools/lz4-compress.js. NFC - #15846
Conversation
cbca7d9 to
6ac078e
Compare
6ac078e to
8b51074
Compare
| var ENVIRONMENT_IS_NODE = typeof process === 'object'; | ||
| var ENVIRONMENT_IS_WEB = typeof window === 'object'; | ||
| var ENVIRONMENT_IS_WORKER = typeof importScripts === 'function'; | ||
| var ENVIRONMENT_IS_SHELL = !ENVIRONMENT_IS_WEB && !ENVIRONMENT_IS_NODE && !ENVIRONMENT_IS_WORKER; | ||
| const arguments_ = process['argv'].slice(2); | ||
| const debug = false; | ||
|
|
||
| if (ENVIRONMENT_IS_NODE) { |
There was a problem hiding this comment.
These are not necessary anymore? It seems we use these variables in other js files..?
There was a problem hiding this comment.
Yeah this script is only ever run under node these days.. and its standalone. This was just copyied and pasted from another script but not needed here.
There was a problem hiding this comment.
Where do we guarantee this only runs on node? I'm not very familiar with this script's usage, but searching lz4-compress in the codebase only turns up tools/file_packager.py, which looks it can run elsewhere too... Also do we not run this on d8 as well?
Anyway, if that's the case, does it give a readable error message when someone tries to run this on other environments?
There was a problem hiding this comment.
This is a completely internal tool just like src/compiler.js and tools/preprocessor.js. These tool may once have worked in non-node environments but today we assume they all run under node only. The code here is completely internal to emscripten and there is exactly one call site.
All of these internal JS scripts require node these days and there is no reason anyone should try to run them under d8 (at least not that I know of).
If a d8 user did try to run this script and saw require() is not a function is should be very obvious that they are trying to run a node script outside of node.
| var bh = b >>> 16; | ||
| var bl = b & 0xffff; | ||
| return (al*bl + ((ah*bl + al*bh) << 16))|0; | ||
| assert = function(x, message) { |
There was a problem hiding this comment.
Other functions in this file have function assert(x, message) { ... form.. Is there a reason why this function is different?
There was a problem hiding this comment.
Yes, I think its because this function needs to be added to the global namespace so that the script that is eval 'ed below had visibility of it.
Its a strange quirk of node and if you just say foo = 3 that creates a new name foo on the global object that is shared.
I have followup PR that will remove the use of eval completely .. but for now this smaller cleanup still relies on eval and the global shared namespace.
| // The path is absolute if the normalized version is the same as the resolved. | ||
| if (!ret && filename != nodePath['resolve'](filename)) { | ||
| filename = path.join(__dirname, '..', 'src', filename); | ||
| ret = nodeFS['readFileSync'](filename); | ||
| } |
There was a problem hiding this comment.
Is this part not necessary anymore?
There was a problem hiding this comment.
No, this was just boilerplate copied from other scripts. Its not needed there.
| var ENVIRONMENT_IS_NODE = typeof process === 'object'; | ||
| var ENVIRONMENT_IS_WEB = typeof window === 'object'; | ||
| var ENVIRONMENT_IS_WORKER = typeof importScripts === 'function'; | ||
| var ENVIRONMENT_IS_SHELL = !ENVIRONMENT_IS_WEB && !ENVIRONMENT_IS_NODE && !ENVIRONMENT_IS_WORKER; | ||
| const arguments_ = process['argv'].slice(2); | ||
| const debug = false; | ||
|
|
||
| if (ENVIRONMENT_IS_NODE) { |
There was a problem hiding this comment.
Where do we guarantee this only runs on node? I'm not very familiar with this script's usage, but searching lz4-compress in the codebase only turns up tools/file_packager.py, which looks it can run elsewhere too... Also do we not run this on d8 as well?
Anyway, if that's the case, does it give a readable error message when someone tries to run this on other environments?
| var bl = b & 0xffff; | ||
| return (al*bl + ((ah*bl + al*bh) << 16))|0; | ||
| assert = function(x, message) { | ||
| if (!x) throw new Errror(message); |
There was a problem hiding this comment.
Looks like one too many rs in this Error?
3 participants
Followup to #15836