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

Package is very hard to use, nullability semantics in particular #1469

Open
matanlurey opened this issue Mar 20, 2024 · 2 comments
Open

Package is very hard to use, nullability semantics in particular #1469

matanlurey opened this issue Mar 20, 2024 · 2 comments
Assignees
Labels
feature-candidate This issue might result in a feature to be implemented suggestion New feature or request

Comments

@matanlurey
Copy link

matanlurey commented Mar 20, 2024

I want to mention I think this package is awesome, and saves me tons of work on hobby projects. Kudos.

That being said, the API state of the project leaves a lot to be desired. Some suggestions:

  • Consider adding a .pickFile with semantics that are less confusing. The default for .pickFiles is ironically allowMultiple: false, but that also means I need to write overly defensive code just to see if a file was actually picked:

    final result = await FilePicker.platform.pickFiles(...);
    if (result == null || result.files.isEmpty) {
      return;
    }
    final file = result.files.single;
    // ... I finally have a file.

    I could imagine a much better API:

    final file = await FilePicker.platform.pickFile(...):
    if (file == null) {
      return;
    }
    // ... I have a file

    In general, I think named arguments are over-used in Dart. They are great for things where users might need to customize (at runtime) arguments, but are less great for different code branches (it's rare that you will want to, dynamically, pick a single or multiple files - usually that's static in business logic).

  • Document how PlatformFile is intended to be used most of the time. It has three properties that are all nullable (path, bytes, readStream) and 0 documentation explaining why any of these would be null, or what is even intended to be used. Hitting null assertions at runtime sucks.

    In addition, the presence of bytes and readStream is really confusing - the latter makes it seem like I can read from the file asynchronously, but the former synchronously (and perhaps it's even pre fetched, because it's stored as final Uint8List? bytes. Which one is it? Why do I need both?

I have more suggestions, but these are the two that hit me in every project I start with this package.

@matanlurey matanlurey added the suggestion New feature or request label Mar 20, 2024
@miguelpruivo miguelpruivo reopened this Mar 26, 2024
@miguelpruivo
Copy link
Owner

Hi, thank you for the suggestion but you are actually saving just 1 loc with that change vs. the effort that has to be put in order to refactor the API at this point, specially having support for all the platforms. The pickFiles supports multiple properties that are quite verbose and somewhat easy to understand, specially because they are very well documented and with examples on the Wiki.

The way the API was made was to make it as much as seamless experience between the different platforms and not only on mobile, so the devs don't have to call different API's for each like:

if(kIsWeb){
// Pick in a way
} else if(Platform.isIOS){
// Use another
}

Specially when multiple devs support multiple platforms at once.

As for your last points asking for the documentation, I'm not sure if you checked the Wiki where you have the explanation with examples for most of the parameters, as well as the PlatformFile itself where all the properties have docs (regardless of some of them being self-explanatory).

Don't take this the wrong way, but trust me, in the early days of the project I took a lot of time thinking and re-thinking the best way this could be done to seamless support multiple platforms with the less struggle for the dev. It might not be perfect but I think it's solid enough and it works following the Flutter and Dart principles.

In addition, the presence of bytes and readStream is really confusing - the latter makes it seem like I can read from the file >asynchronously, but the former synchronously (and perhaps it's even pre fetched, because it's stored as final Uint8List? >bytes. Which one is it? Why do I need both?

A stream of data is completely different of an array of bytes. The purpose of both is kinda self-explanatory and you have the info in the properties of the bytes for example, but anyway here it is:

  • Bytes: The file is immediately loaded into memory so you have the bytes ready to use, this means that a 1024 bytes file, will have all of those bytes allocated in memory (hence using it) and you are ready to use them as soon as you pick the files.
  • Stream: You open a stream to your file descriptor and you read the data in chunks instead of the whole file at once. You can process this chunks, for instance, to send it to a server gradually. Imagine the same 1024 bytes file, instead of having all at once ready to use, you can read in chunks of 64 kb and upload it or do whatever you want with it until the file is fully read. This is particular useful if device is low-end and hasn't much memory or your dealing with big files.

Again, don't take me wrong, but at this point there's no ETA for a foundational API change so I'll close this. Like you, I'd like to see a few more methods that could save some loc, but this needs to be well thought about. If later on this comes as topic, I'll be happy to re-open it.

@navaronbracke
Copy link
Collaborator

I have to agree with @matanlurey here. We have a bunch of things that can be improved in our API that could make things better.

The first of which is getting rid of the monstrosity that is FilePickerWeb in the public API, which forces people to do Platform.is checks. Which lead to a lot of people complaining on the web.

@navaronbracke navaronbracke reopened this Apr 18, 2024
@navaronbracke navaronbracke added the feature-candidate This issue might result in a feature to be implemented label Apr 18, 2024
@navaronbracke navaronbracke self-assigned this Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-candidate This issue might result in a feature to be implemented suggestion New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants