packages
packages copied to clipboard
[cross_file] Adding custom sources to feed data to `XFile`
This PR adds a new constructor for XFile (XFile.fromCustomSource) that only takes an implementation of a new abstract class XFileSource. This new class enables users of the cross_file plugin to construct an XFile and have outputs totally managed by the user.
I removed multiple TODOs as the version required to remove them was met.
New tests have also been added for this new constructor for both dart:io and web versions.
NOTE: I got the web version tests to run in a suboptimal environment because of https://github.com/flutter/flutter-intellij/issues/5550, although it seemed like it didn't change the behavior of the tests.
It seems like a move to a federated plugin for cross_file is in the works (https://github.com/flutter/flutter/issues/91869), it could split XFile into multiple classes and break this PR. Should this feature be delayed for the move to a federated or do we merge now and see how it goes then improve it via feedback when the package will be federated?
This PR will enable support for content:// URIs to be implemented with XFiles (It is the first step to close https://github.com/flutter/flutter/issues/147037).
Pre-launch Checklist
- [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
- [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
- [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use
dart format.) - [x] I signed the CLA.
- [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g.
[shared_preferences] - [x] I linked to at least one issue that this PR fixes in the description above.
- [x] I updated
pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes. - [x] I updated
CHANGELOG.mdto add a description of the change, following repository CHANGELOG style. - [x] I updated/added relevant documentation (doc comments with
///). - [x] I added new tests to check the change I am making, or this PR is test-exempt.
- [x] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel on Discord.
(One of) The problems with XFile in its current version, is that it should have been modeled as an interface which people use to read the contents of the file, but then we would have per-platform versions of the creation of the file, so we can use the most appropriate version for each platform (from bytes, from a Blob, from a io:File...)
This PR may help by introducing the extra complexity of the XFileSource (which I think is fine). However, it might have been nicer to migrate the current data sources to internal implementations of the XFileSource, if possible, so you don't have to keep an internal _file and a _source, and compare which one is not null to read from one or the other.
This approach may also fail on the web, where the implementation of Stream openRead is not that great (to put it mildly, there's a TODO linked in the relevant bit of the source).
Anyway, what do you think @stuartmorgan?
it might have been nicer to migrate the current data sources to internal implementations of the
XFileSource
I have successfully kept only the source in the XFile with dart:io, but the web version needs to have access to the blob to read its content which is tricky to get both inside the source and in XFile for saveTo. This could be resolved by making XFileSource require saveTo, but this would mean the implementer would need to deal with differences between web and dart:io.
For example if we construct an XFile using XFile.fromData on web, internally we could have used a shared XFileSource implementation for both web and dart:io but because of that we would need to implement saveTo twice.
This would make the source manage the destination and we don't want that.
From triage: I'll try to look at the design for this sometime in the next week.
From triage: I'll try to look at the design for this sometime in the next week.
@stuartmorgan do ping me if you want to brainstorm over a call!
any update guys?
Sorry for the long delay on this. I think trying to shoehorn another, even more different source into the current design exacerbates the problems XFile already has (e.g., a bunch of fields that don't always make sense), and that we should instead fix the fundamental design first, and then build new sources using that design. The less code we add before reworking the structure, the less we'll potentially have to break.
(triage) closing based on last comment
I'm proposing a redesign of XFile that would make it very easy to add additional platform-specific types to XFile, so developers can pick the best type when creating an XFile (based on platform and data source), and consume those types transparently in a cross-platform way:
- https://github.com/flutter/packages/pull/7591 (needs review)