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

fix: use fs.stat to detect directory index #58

Merged
merged 3 commits into from
Dec 5, 2023

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Dec 5, 2023

fs.ls tells you the type of every directory entry - to do this it loads the first block of each entry to parse the unixfs metadata - this is obviously bad news for very large directories.

Switch to fs.stat which only loads the directory entry we are interested in.

`fs.ls` tells you the type of every directory entry - to do this it
loads the first block of each entry to parse the unixfs metadata -
this is obviously a bad idea for very large directories.

Switch to `fs.stat` which only loads the directory entry we are
interested in.
@achingbrain
Copy link
Member Author

Seeing a transient WebRTC error that will be fixed by libp2p/js-libp2p#2299

Comment on lines +237 to 249
let indexFile: UnixFSStats | null = null

for (const path of this.rootFilePatterns) {
try {
indexFile = await this.fs.stat(cid, {
...options,
path
})

break
} else {
this.log(`Skipping ${file.type} '${file.name}' in root CID because the filename is not in rootFilePatterns: ${this.rootFilePatterns}`)
} catch (err: any) {
this.log('error loading path %c/%s', cid, path, err)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this, I remember you recommended using fs.ls instead of fs.stat

swap fs.stat() async race for root files (index.html, etc) to fs.ls iterator.. seems much faster (and less wasteful)

from #53. But this seems to be an improvement upon the original from https://github.com/ipfs/helia-http-gateway/pull/53/files#diff-685aae38f06a7057b61c9ef9d72086202a6729d8e63aee9395ef750557496d3eL234 for sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it was the wrong call 🤭

@SgtPooki SgtPooki merged commit 285eeea into main Dec 5, 2023
1 of 2 checks passed
@achingbrain achingbrain deleted the fix/use-stat-for-dir-index branch December 5, 2023 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants