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

Split the web support to package dio_browser_adapter #2218

Closed
wants to merge 7 commits into from

Conversation

huanghui1998hhh
Copy link
Contributor

@huanghui1998hhh huanghui1998hhh commented May 17, 2024

#2215 (comment)
Prepare to adapt WASM.

@huanghui1998hhh huanghui1998hhh requested a review from a team as a code owner May 17, 2024 04:30
dio/lib/browser.dart Outdated Show resolved Hide resolved
@AlexV525
Copy link
Member

Oops, pub get seems always implicitly run with the example/ too...

@huanghui1998hhh
Copy link
Contributor Author

A tough problem : (

@@ -4,6 +4,7 @@ import 'dart:typed_data';
import 'package:meta/meta.dart';

import 'adapters/io_adapter.dart'
if (dart.library.js_util) 'adapters/browser_adapter.dart'
if (dart.library.html) 'adapters/browser_adapter.dart' as adapter;

Choose a reason for hiding this comment

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

This line should be removed. ALL references to dart:html and dart.library.html should be removed.

Choose a reason for hiding this comment

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

Actually removing these might require upping the min Dart version to 3.3 which isn't ideal

Copy link
Member

Choose a reason for hiding this comment

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

This line should be removed. ALL references to dart:html and dart.library.html should be removed.

Will this cause WASM to degrade or something related?

Choose a reason for hiding this comment

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

I'm not sure. All I know is the migration docs say to remove them.

Choose a reason for hiding this comment

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

Since this is an import condition it shouldn't cause any issues, but the js_util ones definitely need changed to js_interop as js_util is one of the deprecated web packages

Copy link
Member

Choose a reason for hiding this comment

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

I think the imports are resolved at compile time, so it should be fine. But definitely should switch to the new js_interop package. We need to figure out if it is possible to run our adapter test in WASM, that will tell us that it is working

@@ -4,6 +4,7 @@ import 'dart:typed_data';
import 'package:meta/meta.dart';

import 'adapters/io_adapter.dart'
if (dart.library.js_util) 'adapters/browser_adapter.dart'

Choose a reason for hiding this comment

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

This should be dart.library.js_interop as mentioned here: https://dart.dev/interop/js-interop/package-web#conditional-imports

@@ -25,7 +25,9 @@

import 'dart:async';

import 'compute_io.dart' if (dart.library.html) 'compute_web.dart' as _c;
import 'compute_io.dart'
if (dart.library.js_util) 'compute_web.dart'

Choose a reason for hiding this comment

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

Same here

@@ -3,6 +3,7 @@ import 'dart:async';
import 'adapter.dart';
import 'cancel_token.dart';
import 'dio/dio_for_native.dart'
if (dart.library.js_util) 'dio/dio_for_browser.dart'

Choose a reason for hiding this comment

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

Same here

@@ -15,6 +15,7 @@ import 'headers.dart';
import 'interceptors/imply_content_type.dart';
import 'options.dart';
import 'progress_stream/io_progress_stream.dart'
if (dart.library.js_util) 'progress_stream/browser_progress_stream.dart'

Choose a reason for hiding this comment

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

Same here

@@ -4,6 +4,7 @@ import 'dart:convert';
import 'package:http_parser/http_parser.dart';

import 'multipart_file/io_multipart_file.dart'
if (dart.library.js_util) 'multipart_file/browser_multipart_file.dart'

Choose a reason for hiding this comment

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

Same

@AlexV525
Copy link
Member

AlexV525 commented Jun 9, 2024

Closing in favor of #2223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants