Skip to content

[cross_file] [web] Separate "Save As" implementation details from XFile web class #10396

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.

Sign up for GitHub

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

Closed
raju-muliyashiya wants to merge 1 commit into from
Closed

[cross_file] [web] Separate "Save As" implementation details from XFile web class #10396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/cross_file/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.3.6

* Separate "Save As" implementation details from XFile web class

## 0.3.5

* Fixes a bug where the bytes of an XFile, that is created using the `XFile.fromData` constructor, are ignored on web.
Expand Down
51 changes: 5 additions & 46 deletions packages/cross_file/lib/src/types/html.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import 'dart:convert';
import 'dart:js_interop';
import 'dart:typed_data';

import 'package:meta/meta.dart';
import 'package:web/web.dart';

import '../web_helpers/web_helpers.dart';
Expand Down Expand Up @@ -35,11 +34,9 @@ class XFile extends XFileBase {
int? length,
Uint8List? bytes,
DateTime? lastModified,
@visibleForTesting CrossFileTestOverrides? overrides,
}) : _mimeType = mimeType,
_path = path,
_length = length,
_overrides = overrides,
_lastModified = lastModified ?? DateTime.fromMillisecondsSinceEpoch(0),
_name = name ?? '',
super(path) {
Expand All @@ -57,10 +54,8 @@ class XFile extends XFileBase {
int? length,
DateTime? lastModified,
String? path,
@visibleForTesting CrossFileTestOverrides? overrides,
}) : _mimeType = mimeType,
_length = length,
_overrides = overrides,
_lastModified = lastModified ?? DateTime.fromMillisecondsSinceEpoch(0),
_name = name ?? '',
super(path) {
Expand All @@ -82,12 +77,16 @@ class XFile extends XFileBase {

// MimeType of the file (eg: "image/gif").
final String? _mimeType;

// Name (with extension) of the file (eg: "anim.gif")
final String _name;

// Path of the file (must be a valid Blob URL, when set manually!)
late String _path;

// The size of the file (in bytes).
final int? _length;

// The time the file was last modified.
final DateTime _lastModified;

Expand All @@ -97,17 +96,6 @@ class XFile extends XFileBase {
// (Similar to a (read-only) dart:io File.)
Blob? _browserBlob;

// An html Element that will be used to trigger a "save as" dialog later.
// TODO(dit): https://github.com/flutter/flutter/issues/91400 Remove this _target.
late Element _target;

// Overrides for testing
// TODO(dit): https://github.com/flutter/flutter/issues/91400 Remove these _overrides,
// they're only used to Save As...
final CrossFileTestOverrides? _overrides;

bool get _hasTestOverrides => _overrides != null;

@override
String? get mimeType => _mimeType;

Expand Down Expand Up @@ -203,37 +191,8 @@ class XFile extends XFileBase {

/// Saves the data of this CrossFile at the location indicated by path.
/// For the web implementation, the path variable is ignored.
// TODO(dit): https://github.com/flutter/flutter/issues/91400
// Move implementation to web_helpers.dart
@override
Future<void> saveTo(String path) async {
// Create a DOM container where the anchor can be injected.
_target = ensureInitialized('__x_file_dom_element');

// Create an <a> tag with the appropriate download attributes and click it
// May be overridden with CrossFileTestOverrides
final HTMLAnchorElement element =
_hasTestOverrides
? _overrides!.createAnchorElement(this.path, name)
as HTMLAnchorElement
: createAnchorElement(this.path, name);

// Clear the children in _target and add an element to click
while (_target.children.length > 0) {
_target.removeChild(_target.children.item(0)!);
}
addElementToContainerAndClick(_target, element);
await saveFileAs(this, path);
}
}

/// Overrides some functions to allow testing
// TODO(dit): https://github.com/flutter/flutter/issues/91400
// Move this to web_helpers_test.dart
@visibleForTesting
class CrossFileTestOverrides {
/// Default constructor for overrides
CrossFileTestOverrides({required this.createAnchorElement});

/// For overriding the creation of the file input element.
Element Function(String href, String suggestedName) createAnchorElement;
}
28 changes: 28 additions & 0 deletions packages/cross_file/lib/src/web_helpers/web_helpers.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@
// found in the LICENSE file.

import 'package:web/web.dart';
import '../types/html.dart';

/// Type definition for function that creates anchor elements
typedef CreateAnchorElement =
HTMLAnchorElement Function(String href, String? suggestedName);

/// Override for creating anchor elements for testing purposes
CreateAnchorElement? anchorElementOverride;
Comment on lines +12 to +13
Copy link

@gemini-code-assist gemini-code-assist bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a global variable for testing can lead to test pollution. It's good practice to:

  1. Annotate it with @visibleForTesting to signal its intended use. This requires importing package:meta/meta.dart at the top of the file.
  2. Add a comment reminding developers to clean it up in their tests using tearDown or addTearDown.
/// Override for creating anchor elements for testing purposes.
///
/// This should be reset to `null` in a `tearDown` block to avoid test pollution.
@visibleForTesting
CreateAnchorElement? anchorElementOverride;


/// Create anchor element with download attribute
HTMLAnchorElement createAnchorElement(String href, String? suggestedName) =>
Expand Down Expand Up @@ -35,3 +43,23 @@ Element ensureInitialized(String id) {
bool isSafari() {
return window.navigator.vendor == 'Apple Computer, Inc.';
}

/// Saves the given [XFile] to user's device ("Save As" dialog).
Future<void> saveFileAs(XFile file, String path) async {
// Create container element.
final Element target = ensureInitialized('__x_file_dom_element');

// Create <a> element.
final HTMLAnchorElement element =
anchorElementOverride != null
? anchorElementOverride!(file.path, file.name)
: createAnchorElement(file.path, file.name);

// Clear existing children before appending new one.
while (target.children.length > 0) {
target.removeChild(target.children.item(0)!);
}
Comment on lines +59 to +61
Copy link

@gemini-code-assist gemini-code-assist bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Instead of a while loop to remove child elements, you can use the more modern and concise Element.replaceChildren() method to clear the container.

  target.replaceChildren();


// Add and click.
addElementToContainerAndClick(target, element);
}
2 changes: 1 addition & 1 deletion packages/cross_file/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: cross_file
description: An abstraction to allow working with files across multiple platforms.
repository: https://github.com/flutter/packages/tree/main/packages/cross_file
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+cross_file%22
version: 0.3.5
version: 0.3.6

environment:
sdk: ^3.7.0
Expand Down
11 changes: 3 additions & 8 deletions packages/cross_file/test/x_file_html_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import 'dart:js_interop';
import 'dart:typed_data';

import 'package:cross_file/cross_file.dart';
import 'package:cross_file/src/web_helpers/web_helpers.dart' as helpers;
import 'package:test/test.dart';
import 'package:web/web.dart' as html;

Expand Down Expand Up @@ -147,15 +148,9 @@ void main() {
final html.HTMLAnchorElement mockAnchor =
html.document.createElement('a') as html.HTMLAnchorElement;

final CrossFileTestOverrides overrides = CrossFileTestOverrides(
createAnchorElement: (_, __) => mockAnchor,
);
helpers.anchorElementOverride = (_, __) => mockAnchor;

final XFile file = XFile.fromData(
bytes,
name: textFile.name,
overrides: overrides,
);
final XFile file = XFile.fromData(bytes, name: textFile.name);
Comment on lines +151 to +153
Copy link

@gemini-code-assist gemini-code-assist bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Modifying a global variable (helpers.anchorElementOverride) in a test can lead to test pollution, where one test's state affects another. It's crucial to clean up this state after the test runs. You can use addTearDown to ensure the override is reset.

Additionally, the test may create a DOM element that should also be cleaned up to prevent side effects between tests.

        addTearDown(() {
          helpers.anchorElementOverride = null;
          html.document.querySelector('#$crossFileDomElementId')?.remove();
        });
        helpers.anchorElementOverride = (_, __) => mockAnchor;

        final XFile file = XFile.fromData(bytes, name: textFile.name);


bool clicked = false;
mockAnchor.onClick.listen((html.MouseEvent event) => clicked = true);
Expand Down