Skip to content
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

Expose internal statValues in a consumer friendly way. #19167

Closed
jdalton opened this issue Mar 6, 2018 · 9 comments
Closed

Expose internal statValues in a consumer friendly way. #19167

jdalton opened this issue Mar 6, 2018 · 9 comments
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. wontfix Issues that will not be fixed.

Comments

@jdalton
Copy link
Member

jdalton commented Mar 6, 2018

The fs.stat and fs.statSync methods have access to an internal statValues typed array which holds things like raw mtimeMs values. It would be nice to avoid creating an entire stats object for one-off things like mtimeMs and friends.

Since Node is already tracking these values in a way that makes it easy to pluck them individually what do you all think about splitting each value out into their own method which then accesses the internal statValues array instead of creating a catch-all for all-values as the stat methods do today?

Related: This would also nicely side step the perf issue associated with stat too.

@Fishrock123 Fishrock123 added fs Issues and PRs related to the fs subsystem / file system. feature request Issues that request new features to be added to Node.js. labels Mar 6, 2018
@apapirovski apapirovski added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Apr 12, 2018
@rogelio-o
Copy link

Is anyone working on this? If not, I can try.

@bnoordhuis
Copy link
Member

I don't think we would/should expose it as a public API, it'd be inflexible and hard to maintain. It's also a case of what I call pythonitis, having two or more ways to do the same thing.

(Python's stdlib is rife with them, hence the name.)

@jdalton
Copy link
Member Author

jdalton commented May 25, 2018

Creating an object with lots of instantiated dates to get a single mtime isn't great. It's why there's an internal fast path and why folks have attempted to optimize the external API. Having a way to get the individual bits of data that are already known, without making the operation heavier than needed, would be a win.

@bnoordhuis Do you have any ideas on how to improve that situation?

@jasnell
Copy link
Member

jasnell commented May 25, 2018

I agree with @bnoordhuis that having separate methods for individual stat fields is not a great approach here. If the concern is about always instantiating the various Date objects when they may not be needed, we could always lazy create those by switching to a getter for those fields.

@jdalton
Copy link
Member Author

jdalton commented May 25, 2018

If the concern is about always instantiating the various Date objects when they may not be needed, we could always lazy create those by switching to a getter for those fields.

I believe a lazy approach was attempted before but reverted-out, so if attempted again folks can use the previous try as a guide for potential snags. Trying again sounds rad to.

@jasnell
Copy link
Member

jasnell commented May 25, 2018

I seem to recall something about that also.. I'll see if I can dig up the details.

Another potentially interesting approach would be new stat function variants that allowed you to filter what was returned... e.g.

fs.statOnly('/foo', fs.STAT_MTIME | fs.STAT_SIZE, (err, stat) => {/* ... */});
fs.fstatOnly(fd, fs.STAT_MTIME | fs.STAT_SIZE, (err, stat) => {/* ... */});
fs.lstatOnly('/foo', fs.STAT_MTIME | fs.STAT_SIZE, (err, stat) => {/* ... */});

This would run slightly afoul of @bnoordhuis's anti-pythonitis PoV but it would side-step any backwards compat issues that may exist around lazy-instantiation of the stat Dates.

@jdalton
Copy link
Member Author

jdalton commented May 25, 2018

This would run slightly afoul of @bnoordhuis's anti-pythonitis PoV but it would side-step any backwards compat issues that may exist around lazy-instantiation of the stat Dates

Less API, works around back-compat issues, and accomplishes the same end result is even better 🎉

@bnoordhuis
Copy link
Member

I believe a lazy approach was attempted before but reverted-out, so if attempted again folks can use the previous try as a guide for potential snags.

That was #12818 and #13256 respectively. The plan was to revisit it some time in the future and here we are.

@bnoordhuis
Copy link
Member

I'm going to close this out. It's been open for nearly two years without any movement.

@bnoordhuis bnoordhuis added the wontfix Issues that will not be fixed. label Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. wontfix Issues that will not be fixed.
Projects
None yet
Development

No branches or pull requests

6 participants