-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Properly handle Windows file: URLs on Node.js #30098
Conversation
This adds logic to the JS implementation of URI to determine whether the code is running on Windows under Node.js.
We aren't running Node tests here until dart-lang/sdk#30098 lands.
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.
LGTM if the test is fixed.
static bool get _isWindows => _isWindowsCached; | ||
|
||
static final bool _isWindowsCached = | ||
JS('bool', 'process !== undefined && process.platform == "win32"'); |
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 looks unsafe. Using "process" with no prefix requires "process" to exist, otherwise the access throws.
Maybe use typeof to avoid that:
typeof process != "undefined" && Object.prototype.toString.call(process) === "[object process]" && process.platform === "win32"
(This also strengthens the check for node.js since that's the only known platform that has a object class named "process")
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.
Done.
Remove the workaround for dart-lang/sdk#30098
This adds logic to the JS implementation of URI to determine whether
the code is running on Windows under Node.js.