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

Align file sink and source creation behavior across targets #252

Merged
merged 4 commits into from
Jan 24, 2024

Conversation

fzhinkin
Copy link
Collaborator

For JS, unlike other targets, a file was not opened on file sink/source creation.

For a sink, it was leading to different behavior when the sink was closed without a single byte written to it: on all targets except JS an empty file is created.

For a source, on all targets except JS, an attempt to create a source for a non-existent file is immediately reported as an error, but for JS the error won't be reported until an actual read operation is employed.

This PR aligns JS target's behavior will all other targets.

Fixes #251

That ensures that even if nothing was written, a new empty file will be created on close.

Fixes #251
Align the behavior across all targets
@fzhinkin fzhinkin added the bug label Jan 15, 2024
@@ -92,6 +92,20 @@ internal class FileSource(private val path: Path) : RawSource {
private var buffer: dynamic = null
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why aren't you using the Buffer type from kotlin-wrappers for the buffer instances?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a JS buffer returned by the fs.readSync and it's incompatible with kotlinx.io.Buffer.

The way it all works could be improved (by removing that buffer altogether and using an array from a buffer passed to the readAtMostTo call as a destination buffer supplied to fs.readSync).

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks! Asked because even the require('buffer') at the top of the file is untyped and was wondering why.

throw FileNotFoundException("File does not exist: $path")
}
val fd = try {
fs.openSync(path.path, "r") as Int
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally when I know the specific numeric type, I prefer to use an unsafeCast.
Casting with as will generate additional code paths and deeper call stack for pretty much no reasons.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing to this. I don't actually care about the exact type here, so it's fine to leave it as dynamic.

@fzhinkin fzhinkin marked this pull request as ready for review January 15, 2024 11:05
@fzhinkin fzhinkin linked an issue Jan 15, 2024 that may be closed by this pull request
@fzhinkin fzhinkin self-assigned this Jan 15, 2024
@fzhinkin fzhinkin added the fs label Jan 18, 2024
core/js/src/files/PathsJs.kt Outdated Show resolved Hide resolved
core/js/src/files/PathsJs.kt Show resolved Hide resolved
core/js/src/files/PathsJs.kt Outdated Show resolved Hide resolved
@fzhinkin fzhinkin merged commit 78d0fb1 into develop Jan 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SystemFileSystem.sink does not create empty file on NodeJs target
3 participants