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(node): implement fs.statfs() #22862
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.
LGTM, @littledivy any objections?
@@ -12,6 +12,7 @@ export const ArrayPrototypeSlice = (that, ...args) => that.slice(...args); | |||
export const ArrayPrototypeSome = (that, ...args) => that.some(...args); | |||
export const ArrayPrototypeSort = (that, ...args) => that.sort(...args); | |||
export const ArrayPrototypeUnshift = (that, ...args) => that.unshift(...args); | |||
export const BigInt = globalThis.BigInt; |
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 BigInt
not available in primordials
from ext:core/mod.js
? If so we should fix it 😬
ext/node/ops/fs.rs
Outdated
#[allow(clippy::disallowed_methods)] | ||
let path = Path::new("Cargo.toml").canonicalize().unwrap(); |
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.
Wouldn't canonicalize()
fail if Cargo.toml
is not present?
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 sure why we need this. The user path
argument is shadowed by this hardcoded path to Cargo.toml
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 must have copy-pasted this from my windows playground repo... that's frustrating there was no lint error.
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.
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!
Closes #21139.