Refactor FileReader to fix side-effect and provide extra context #279
Conversation
|
Looks great, thanks @sloshy! I'm going to read it properly tomorrow when I'm slightly more awake, but I've kicked the build off in the meantime. 😃 |
| @@ -16,103 +18,88 @@ import scala.scalajs.js.typedarray | |||
| */ | |||
| object FileReader: | |||
|
|
|||
| val readImageCast: Result[js.Any] => Result[String] = | |||
There was a problem hiding this comment.
question: should these *Cast vals be private?
There was a problem hiding this comment.
I like this improvement of extracting them out in general, but yes I agree that they don't belong on the public interface. 👍
There was a problem hiding this comment.
Have been rather busy recently but I'll make a quick commit for this later tonight (or try to)
There was a problem hiding this comment.
Thanks @sloshy, I've been busy too, it happens. 🙂
Don't worry about it. I'm going to go ahead and merge this and then I'll quickly make the change - your efforts are appreciated! 🙏
There was a problem hiding this comment.
Ahh sorry things have been a little hectic over here. You're the best!
| val task = maybeGetFile.flatMap { | ||
| case None => Future.successful(Result.NoFile("No files on specified input")).pure[F] | ||
| case Some(file) => | ||
| Async[F].delay { |
There was a problem hiding this comment.
thought: the sequence of callback -> promise -> future -> f seems like it should be Async[F].async. On top of the conversions having extra cost, it doesn't handle cancellation (which might result in memory leaking the event listener/promise in case the command gets cancelled).
It would probably be beneficial to rewrite this as Async[F].async and remove the event listener in case of interruption. note, I can't remember if command effects actually are interruptible.
There was a problem hiding this comment.
I can't remember if command effects actually are interruptible
In practice, not really. You can do it but you have to work hard to make it happen, it isn't just easily available. It is one of those topics that comes up a lot on the backend but... my hypothesis is that it's less important on the frontend, and that if you have one of those cases where it really does matter then you won't mind doing the work.
So it's a tradeoff. In this arch style, precise effect management is more difficult, but the overall code and life cycle is (IMO) simpler and cleaner (Discussions on scaling not withstanding 😛).
There was a problem hiding this comment.
I agree that there is a good argument for converting to Async[F].async, however I don't think that's @sloshy's problem. This code is an improvement, so no need to hold up the PR any longer.
But it's a good suggest, and I've captured it as an issue.
There was a problem hiding this comment.
LGTM @sloshy, thanks for the work. Will hang on before merging in case anyone wants to comment. If you feel like applying the suggesting to make the extracted functions private, that would be lovely, but otherwise I'm happy. Thanks again!
| @@ -16,103 +18,88 @@ import scala.scalajs.js.typedarray | |||
| */ | |||
| object FileReader: | |||
|
|
|||
| val readImageCast: Result[js.Any] => Result[String] = | |||
There was a problem hiding this comment.
I like this improvement of extracting them out in general, but yes I agree that they don't belong on the public interface. 👍
| val task = maybeGetFile.flatMap { | ||
| case None => Future.successful(Result.NoFile("No files on specified input")).pure[F] | ||
| case Some(file) => | ||
| Async[F].delay { |
There was a problem hiding this comment.
I can't remember if command effects actually are interruptible
In practice, not really. You can do it but you have to work hard to make it happen, it isn't just easily available. It is one of those topics that comes up a lot on the backend but... my hypothesis is that it's less important on the frontend, and that if you have one of those cases where it really does matter then you won't mind doing the work.
So it's a tradeoff. In this arch style, precise effect management is more difficult, but the overall code and life cycle is (IMO) simpler and cleaner (Discussions on scaling not withstanding 😛).
| val task = maybeGetFile.flatMap { | ||
| case None => Future.successful(Result.NoFile("No files on specified input")).pure[F] | ||
| case Some(file) => | ||
| Async[F].delay { |
There was a problem hiding this comment.
I agree that there is a good argument for converting to Async[F].async, however I don't think that's @sloshy's problem. This code is an improvement, so no need to hold up the PR any longer.
But it's a good suggest, and I've captured it as an issue.
Found this unexpected behavior when using the FileReader by defining a
Cmdas a Scalaval.The code to read the
dom.Filefrom an input element by ID executes eagerly, before returning the final suspendedCmdfor reading a file. This means that if you use this helper in your code as avalit will get the result at definition time, and not when you actually expect theCmdto execute.To fix this, I made the following changes:
readFilenow takes a parameter to get thedom.Fileinput so that it can be provided multiple ways.Result.NoFiletype that is a variant ofResult.Errorbut providing the additional context that there is no file specified. This can be useful for more precise application error messages in addition to making refactoring easier.castfunctions in each helper method to their own values so the file size is smaller and it's easier to keep these in sync if their behavior should ever change.Hope this change meets your standards and thanks again for the wonderful project!