-
Notifications
You must be signed in to change notification settings - Fork 72
Align file sink and source creation behavior across targets #252
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
Conversation
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
| } | ||
|
|
||
| internal class FileSource(private val path: Path) : RawSource { | ||
| private var buffer: dynamic = null |
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.
Just curious, why aren't you using the Buffer type from kotlin-wrappers for the buffer instances?
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.
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).
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.
Got it, thanks! Asked because even the require('buffer') at the top of the file is untyped and was wondering why.
core/js/src/files/PathsJs.kt
Outdated
| throw FileNotFoundException("File does not exist: $path") | ||
| } | ||
| val fd = try { | ||
| fs.openSync(path.path, "r") as Int |
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.
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.
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.
Thanks for pointing to this. I don't actually care about the exact type here, so it's fine to leave it as dynamic.
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