[RFC] [cross_file] New architecture. #7591

ditman · 2024-09-06T02:42:27Z

This PR attempts to solve the current issues of XFile by slightly re-architecting it.

In this PR:

XFile is now an interface that does not have any code, except for the two "legacy" constructors that are maintained (but deprecated) for backwards-compatibility reasons.
Each underlying data structure contains a super simple implementation of the interface.
XFile instances are now created from the platform-specific (native vs web) factories.

This does not remove saveAs from the XFile interface.

Note

This is a breaking change because I've made the old XFile constructors deprecated (and throwy) but if we want to, we could possibly re-implement the "old" constructors with the new implementations and a few conditional imports?

This is not a breaking change anymore. Old tests continue passing (test/legacy*), and new tests contain minor changes.

Anyway, just a RFC!

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

ditman · 2024-09-06T02:47:02Z

/cc @stuartmorgan this is my idea for the "next" version of the x-files. PTAL and let me know what you think, or if you want this to go in another direction :)

packages/cross_file/lib/src/implementations/blob_x_file.dart

ditman · 2024-09-06T02:51:28Z

packages/cross_file/lib/src/implementations/io_bytes_x_file.dart

+
+  @override
+  Future<void> saveTo(String path) async {
+    final File fileToSave = File(path);


This is lifted from the old implementation but... should the File be created from path + this.name?

packages/cross_file/lib/src/implementations/web_bytes_x_file.dart

navaronbracke · 2024-09-18T06:29:17Z

packages/cross_file/lib/src/implementations/io_x_file.dart

+
+/// A CrossFile backed by a dart:io [File].
+///
+/// (Mobile-only).


Don't forget desktop :)

Suggested change

/// (Mobile-only).

/// This is only supported on non-web platforms.

navaronbracke · 2024-09-18T06:34:24Z

packages/cross_file/lib/src/implementations/blob_x_file.dart

+  /// The [path] variable is ignored.
+  @override
+  Future<void> saveTo(String path) async {
+    // Save a Blob to file...


This comment seems a bit redundant.

navaronbracke · 2024-09-18T06:36:41Z

packages/cross_file/README.md

 objects and their URLs.

 It seems that Safari hangs when reading Blobs larger than 4GB (your app will stop
 without returning any data, or throwing an exception).

-This package will attempt to throw an `Exception` before a large file is accessed


Just to be sure, was this comment removed, because we now refer to the browser's behavior? Or was this fixed in recent versions of Safari?

We don't explicitly throw the "4 Gigabyte" exception; the browser may fail in unexpected ways (depending on what we end up doing with the bytes of the file)

navaronbracke · 2024-09-18T06:43:58Z

packages/cross_file/README.md


-Example:
+In order to instantiate a new `XFile`, import the correct factory class,
+either from `package:cross_file/native/factory.dart` (for native development) or


Small drive-by comment:

Since the two implementations now have platform-specific imports (dart:io or package:web)
users who take the imports at face value might run into compilation issues, because they lack a conditional import?

Can we provide guidance on how to avoid this? With the legacy API we had the File or Blob types as internals and not on the public interface, which allowed for the plugin to provide a conditional import shim.

@navaronbracke thanks for the comment! This is a very interesting problem, for which I don't have a great answer. The TL;DR/Guidance is:

Are you reading XFiles? Use the XFile interface only.

Are you creating XFiles?

In mobile? Use the native/factory.dart

In web? Use the web/factory.dart

Each factory exposes constructors that are platform-specific, and we really don't have a cross-platform file constructor.

What problems do you think people are going to have? Is this a documentation problem, or a design issue?

If people take the comment at face value and do import "package:cross_file/web/factory.dart"; directly, they will run into errors like library dart:js_interop is not available on this platform. Since they don't directly import dart:js_interop, this might be confusing to new users.

I think this is merely a documentation issue. We should put emphasis on:

If this is in a platform-specific plugin implementation, you can probably import it directly (as the plugin system only uses the platform implementations of your plugin on the correct platforms)

If this is in a Flutter app, you'll have to use a conditional import on if (dart.library.js_interop) ... and you probably need an extra abstraction, since the web-only types (i.e. Blob) cannot be imported in native implementations (for the same reason why the plugin cannot provide the conditional import here)

OK, if it's a documentation issue, it's probably easier to fix (LOL famous last words :P)

Do you have any ideas with how this comment would be clearer? What would you like to read?

(I'll try to come up with something more specific as well!)

Hmm, the main point is that the compiler itself would want you to use a conditional import.
So perhaps something like:

"In order to instantiate a new XFile, import the correct factory class, through a conditional import.
Using a conditional import is required to use types that are only available on the current platform.

import 'package:cross_file/native/factory.dart' if (dart.library.js_interop) 'package:cross_file/web/factory.dart'

"

The wording is just a suggestion, so feel free to tweak it.

Hmm, wait. Since the API for both factories is divergent, would a user that uses this conditional import know at compile time if they could access the fromFile() (both with a dart:io or package:web File) or the fromBlob()/fromObjectUrl() methods? I think that Dart will only provide the base type XFileFactory, but I think you don't get a guarantee for the available methods.

Perhaps we can fix that by having the interface like:

XFileFactory is the base type that is exported through a conditional export. It provides all the methods that do not include platform-specific types (so only fromPath/fromBytes/fromObjectUrl)

WebXFileFactory and NativeXFileFactory implement that interface

WebXFileFactory provides the fromBlob/fromFile implementations for package:web types

NativeXFileFactory provides the fromFile implementation for dart:io

unsupported methods on a platform just throw UnsupportedError

we direct users to use kIsWeb and/or a type check on the factory

since we do the conditional export, the comment above can stay unchanged, no need to adjust the wording when we provide the export ourselves. We'll have to add guidance for checking which factory is provided instead (simple type check should do)

Thanks for the clarification @navaronbracke! I see what you mean! You want the XFileFactory to be cross-platform as well!

IMO the only cross-platform class from this package is the XFile interface! ¹

If your cross-platform App needs to "create XFiles", you'll end up with some platform-aware code (because as you said, you can't import web stuff in native code), like this:

// Your app's main.dart import 'src/get_file/get_file.dart'; // Platform-aware, see below // ... final XFile myFile = await getFile(/* config? data? */);

With a directory like this:

src/ get_file/ get_file.dart get_file_web.dart get_file_native.dart

Your get_file.dart could conditionally export the right file for the platform; like:

// get_file.dart export "get_file_native.dart" if (dart.library.js_interop) "get_file_web.dart";

So each implementation file can use the platform-specific factories:

get_file_native.dart get_file_web.dart

import 'package:cross_file/native/factory.dart'; XFile getFile(/* config? data? */) async { // Do the right thing for native here, maybe // we write the data to a File in a temp dir // and then: return XFileFactory.fromFile(tmpFile); }

import 'package:cross_file/web/factory.dart'; XFile getFile(/* config? data? */) async { // Do the right thing for the web here, // maybe we grab bytes, put them into a // Blob and do: return XFileFactory.fromBlob(dataBlob); }

Maybe the confusing part is calling the XFileFactory the same on both native and web? Should we just have NativeXFileFactory and WebXFileFactory to make the separation clearer, and signal that we never expect a shared interface across both classes?

It provides all the methods that do not include platform-specific types (so only fromPath/fromBytes/fromObjectUrl)

This is why I think having a base class for the XFileFactory is more trouble than what it's worth. Unless I'm missing something, for this approach to work, the XFileFactory needs to be an actual instance that must be replaced at run-time (similar to a federated plugin?).

Since we can't expose web-only types through this common API, users would still need to conditionally import the native/web types to cast the instance to the one that has the platform specific methods. And if you're conditionally importing, just import the right XFileFactory static class? 😛

Footnotes

This is similar to the Client interface from package:http that has multiple platform-specific implementations coming from different packages ↩

stuartmorgan · 2024-09-18T18:42:16Z

Quick note to say I haven't forgotten about this, I just haven't had time to fully context switch to give this the attention it will need. I'll try to do it within the next week.

…mplementations.

…ors.

…ing the Blob.

stuartmorgan

I'll try to do it within the next week.

😬 Time is an illusion, review time doubly so.

I like this a lot; if we don't want to jump to a breaking change (although I'm not 100% sure this isn't breaking in some obscure ways; we should probably do a quick check of GitHub to see if anyone is subclassing the old structure in any way that isn't compatible and if not call it non-breaking) this looks like a solid implementation.

I do wish we could do static extensions on XFile itself, but since we can't I do like your factory class approach more than what I had originally envisioned which was people interacting directly with subclass constructors. The way your version makes the subclass hidden implementation details that we can adjust over time is much better than that.

Thanks for tackling this, and sorry it took me so long to review!

stuartmorgan · 2024-11-08T00:01:52Z

packages/cross_file/README.md


-Example:
+In order to instantiate a new `XFile`, import the correct factory class,
+either from `package:cross_file/native/factory.dart` (for native development) or


It seems like we could just do this automatically from cross_file.dart with conditional imports. The consuming code wouldn't be cross-platform because the factories are specific, but it wouldn't need to do an extra import.

The problem with conditional exports/imports is that the analyzer freaks out when the APIs are different, which in this case they are (see this comment). Or do you have something else in mind?

stuartmorgan · 2024-11-08T00:09:19Z

packages/cross_file/lib/src/x_file.dart

+
+  /// Get the path of the picked file.
+  ///
+  /// **Not all `XFile` instances have a `path`.**


It seems like we should bite the bullet and do a breaking change to make this and name nullable. But I guess if we can do the rest as a non-breaking change we could make that change later instead, at the same time that we remove the deprecated methods, giving clients the option of supporting a major version range and decreasing conflicts.

I think the changes in this package are sufficiently big to do a breaking change, with all the changes we might want.

I just did a non-breaking change "because I could", but as you said above, it might be "subtly breaking elsewhere".

How do you feel about making this a breaking change? There would be extra work in revisiting the consumers of this package and fixing those (which would also need a major version bump because they tend to re-export XFile as well :S)

I don't mind tackling that as well, LMK if you're okay with the change!

There would be extra work in revisiting the consumers of this package and fixing those (which would also need a major version bump because they tend to re-export XFile as well :S)

Yes, working that through the ecosystem could be kind of rough. I think since your version doesn't make any real structural compromises to make a probably-not-breaking change, I want to dig into usage a bit. My gut is that nobody is doing complex subclassing that would be affected by it, but I want to try to validate that against at least the public code I can find, and especially some popular packages. If it look like it'll be safe (e.g., nobody is ever subclassing any of the classes), then we can do this version and make things easier on everyone.

There's some subclassing going on, most of it to add extra attributes/methods to the underlying XFile instance (they could wrap the underlying XFile and add stuff to it, or use extension methods I think?), one that could maybe migrated to one of the new web constructors, one used to store the file in "Hive", and one that is importing from src (this is illegal!)

extends XFile (search)

https://github.com/MixinNetwork/flutter-plugins/blob/b2beb15590f7b6c38e650ae501923b9bd3e7849c/packages/desktop_drop/lib/src/drop_item.dart#L5 (adds properties to XFile)

https://github.com/vidklopcic/flutter_persistent_socket/blob/1f7dfd216e9360c4d6bfb7aad8b29c0a897d6d22/lib/uploader/file_picker/file_picker_web.dart#L30 (BlobXFile.fromFile?)

https://github.com/slatei/asset_manager/blob/ba5b362b3c6ad26986b6448750d245d4823e3c17/lib/shared/hashed_file.dart#L5 (adds properties to XFile)

https://github.com/Wayaer/fl_assets_picker/blob/a66c525a9d9d937c1f31750b8b9f2d76513fa6ae/fl_image_picker/lib/src/extension.dart#L7 (adds properties to XFile)

https://github.com/diego78795/contact_app/blob/0d224aad5fb31353336569408133ef9951223003/lib/adapters/image_adapter.dart#L4 (extension method on XFile?)

https://github.com/correiarangel/camera_poc/blob/7d9e37549aa90001dfd97ed72bf65c37002c5dff/lib/core/shared/services/camera/library/z_xfile.dart#L3 (proxies XFile)

https://github.com/BreX900/cross_file_picker/blob/02dec6132bf5dacb020a6592d847d219d3aeded8/lib/src/x_file_stream.dart#L8 (imports from 'src')

https://github.com/monzir3bdo/task/blob/d2d4f906ed34bdaa6e92cc73a7b83512a1f66509/lib/features/products/data/models/photo_model.dart#L9 (could use the new interface)

implements XFile (search)

All seems to be test code.

(extension ...) on XFile (search)

Everything should work (?)

I could create issues in their respective owner's repos to point them to this PR so they can see what's coming before we land it?

That looks like we would break existing packages, unfortunately. Warning people in advance isn't enough for that, we'd need to version accordingly.

So I think the options are:

Do a major version change and deal with the resulting pain.

Make the new interface a super-interface of XFileBase, and let individual packages adopt that incrementally (although that would still be a breaking change for the packages that export it in their API).

The main drawback to 2 is that we need to change the name of the class everyone uses, and we would be stuck with the legacy bits for however long we want to support both.

I think I prefer 1.

With the current implementation, the package is backwards-compatible for ourselves, so the transition for our packages should be relatively painless (and we could allow a version range).

(Unless we want to do the nullability changes at the same time, of course :))

If we're going to do a major version change, that's our chance to fix nullability; if we don't do it then we probably never will.

OK, I'll do all the incompatible changes in one go. Can you think of anything else? Maybe remove the saveTo method from the class, and make it a helper instead?

stuartmorgan · 2024-11-08T00:11:15Z

packages/cross_file/lib/src/x_file.dart

+
+  /// The MIME-type of the file.
+  ///
+  /// **Not all `XFile` instances have a `mimeType`.**


We don't need to say this since it's nullable.

ditman requested a review from stuartmorgan September 6, 2024 02:42

github-actions bot added p: cross_file platform-web labels Sep 6, 2024

ditman commented Sep 6, 2024

View reviewed changes

packages/cross_file/lib/src/implementations/blob_x_file.dart Outdated Show resolved Hide resolved

ditman commented Sep 6, 2024

View reviewed changes

packages/cross_file/lib/src/implementations/web_bytes_x_file.dart Outdated Show resolved Hide resolved

ditman mentioned this pull request Sep 11, 2024

[file_selector] MIME Type Returning Null from Picked Markdown File on Web flutter/flutter#132498

Open

2 tasks

navaronbracke reviewed Sep 18, 2024

View reviewed changes

ditman added 16 commits October 9, 2024 14:20

Remove old types

c1a7d31

Make XFile an interface and add different implementations.

f4f504e

Add library entrypoints.

285d994

Update tests with new factory methods/behavior.

6bfc7ac

Update documentation.

4ae170f

update changelog and pubspec

0f8d772

Add legacy tests.

b0a34e8

Add legacy factory to re-write the legacy constructors with the new i…

8ebf101

…mplementations.

Remove nullability of path and name. Reimplement deprecated construct…

89f9b8f

…ors.

Make name and path non-nullable.

c574bdb

Redesign BlobXFile so we can have a non async fromObjectURL constructor.

a0e254b

Split downloadBlob method so we can download objectUrls without fetch…

45d9f06

…ing the Blob.

Use the new ObjectUrlBlobXFile in the web XFileFactory.

1c3b4cf

Adapt new tests to latest API changes.

14001c9

Mark update as backwards-compatible, which it seems to be!

e0fa2e6

Add missing license header.

0873768

ditman force-pushed the cross-file-next branch from 560405c to 0873768 Compare October 9, 2024 21:24

ditman mentioned this pull request Nov 7, 2024

[cross_file] Adding custom sources to feed data to XFile #6625

Closed

11 tasks

Merge branch 'main' into cross-file-next

dbaabeb

stuartmorgan reviewed Nov 8, 2024

View reviewed changes

helomri mentioned this pull request Dec 11, 2024

Support for Content:// Uri to access files on shared storage flutter/flutter#147037

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] [cross_file] New architecture. #7591

[RFC] [cross_file] New architecture. #7591

ditman commented Sep 6, 2024 •

edited

Loading

ditman commented Sep 6, 2024

ditman Sep 6, 2024

navaronbracke Sep 18, 2024

navaronbracke Sep 18, 2024

navaronbracke Sep 18, 2024

ditman Sep 23, 2024

navaronbracke Sep 18, 2024

ditman Sep 23, 2024

navaronbracke Sep 24, 2024

ditman Sep 24, 2024

navaronbracke Sep 25, 2024

navaronbracke Sep 25, 2024 •

edited

Loading

ditman Sep 25, 2024

stuartmorgan commented Sep 18, 2024

stuartmorgan left a comment

stuartmorgan Nov 8, 2024

ditman Nov 8, 2024 •

edited

Loading

stuartmorgan Nov 8, 2024

ditman Nov 8, 2024

stuartmorgan Nov 8, 2024

ditman Nov 8, 2024

stuartmorgan Nov 8, 2024 •

edited

Loading

ditman Nov 8, 2024 •

edited

Loading

stuartmorgan Nov 11, 2024

ditman Nov 11, 2024

stuartmorgan Nov 8, 2024

	/// (Mobile-only).
	/// This is only supported on non-web platforms.

[RFC] [cross_file] New architecture. #7591

Are you sure you want to change the base?

[RFC] [cross_file] New architecture. #7591

Conversation

ditman commented Sep 6, 2024 • edited Loading

Pre-launch Checklist

ditman commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navaronbracke Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

stuartmorgan commented Sep 18, 2024

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ditman Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

ditman Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ditman commented Sep 6, 2024 •

edited

Loading

navaronbracke Sep 25, 2024 •

edited

Loading

ditman Nov 8, 2024 •

edited

Loading

stuartmorgan Nov 8, 2024 •

edited

Loading

ditman Nov 8, 2024 •

edited

Loading