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: fix handle paths in FSA entries iterator #1019

Merged
merged 2 commits into from
Mar 31, 2024

Conversation

quickgiant
Copy link
Contributor

Hi! Thanks for working on this library. The File System Access API adapter has been helpful for testing a project I'm working on.

I noticed in one of my test suites that when using file handles retrieved from the NodeFileSystemDirectoryHandle.entries() async iterator, the getFile() method would throw an ENOENT error, with the message suggesting it was trying to read the file using just the name instead of the full path.

This appears to be a typo in the implementation, so I changed it to pass the full path when constructing file handles and added an assertion to the relevant test case that checks that the file entries can be read.

Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Thanks! Would you mind adding an equivalent assertion to the directory example too?

@quickgiant
Copy link
Contributor Author

quickgiant commented Mar 31, 2024

Yeah I can do that. Is it okay if it checks the __path property? I was trying to avoid writing implementation-specific assertions, but I'm not sure there's another good way to check the path for directory entries.

Edit: Added another test case checking __path and discovered that separators were being added twice due to there already being a trailing slash in the base path. Fixed that bug as well and will update the PR title.

@quickgiant quickgiant changed the title fix: fix file handle path in FSA entries iterator fix: fix handle paths in FSA entries iterator Mar 31, 2024
@G-Rath
Copy link
Collaborator

G-Rath commented Mar 31, 2024

I'm not too familiar with this side of the library so couldn't say off the top of my head, but what I was meaning was just the same instanceof assertion you've added in this PR (I'm assuming there's an equivalent Directory type class like there is File, but if that's not the case we can skip this)

@quickgiant
Copy link
Contributor Author

quickgiant commented Mar 31, 2024

Yeah there's no equivalent to getFile(). The FileSystemDirectoryHandle class only includes the async iterator methods, getters for child handles, and a method to remove (delete) entries.

@G-Rath G-Rath merged commit b8905eb into streamich:master Mar 31, 2024
9 of 10 checks passed
github-actions bot pushed a commit that referenced this pull request Mar 31, 2024
## [4.8.1](v4.8.0...v4.8.1) (2024-03-31)

### Bug Fixes

* fix handle paths in FSA entries iterator ([#1019](#1019)) ([b8905eb](b8905eb))
@G-Rath
Copy link
Collaborator

G-Rath commented Mar 31, 2024

This has gotten released as v4.8.1 on npm but it looks like the GitHub credentials for semantic release are broken so it hasn't added the release or posted it's usual comment here.

@streamich can you update the GitHub token for ci? seems this happened for the last release too

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