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

Adapt dart2wasm #2215

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Adapt dart2wasm #2215

wants to merge 12 commits into from

Conversation

huanghui1998hhh
Copy link

New Pull Request Checklist

  • I have read the Documentation
  • I have searched for a similar pull request in the project and found none
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

@huanghui1998hhh huanghui1998hhh requested a review from a team as a code owner May 15, 2024 09:36
@huanghui1998hhh huanghui1998hhh changed the title Compatible dart2wasm Adapt dart2wasm May 15, 2024
@kuhnroyal kuhnroyal added the p: dio Targeting `dio` package label May 15, 2024
Copy link
Member

@kuhnroyal kuhnroyal left a comment

Choose a reason for hiding this comment

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

Thanks!
Left some questions, I am generally not sure about min SDK support for this and if the tests work. We might need some test for the new header function.

dio/pubspec.yaml Outdated
@@ -24,6 +24,7 @@ dependencies:
http_parser: ^4.0.0
meta: ^1.5.0
path: ^1.8.0
web: ^0.5.1
Copy link
Member

Choose a reason for hiding this comment

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

What is the min SDK requirement to use the web package?

Copy link
Member

Choose a reason for hiding this comment

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

Seems to be 3.1.0

Copy link
Author

Choose a reason for hiding this comment

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

Seems to be 3.1.0

Maybe 3.3.0?

dio/lib/src/adapters/browser_adapter.dart Outdated Show resolved Hide resolved
dio/lib/src/adapters/browser_adapter.dart Outdated Show resolved Hide resolved
@kuhnroyal
Copy link
Member

Looks like we have to move to Dart 3.1.0 for this.

@AlexV525
Copy link
Member

What about we split the Web support as a package and publish it first, then make a major version upgrade for wasm?

@kuhnroyal
Copy link
Member

Could work but that would also imply a breaking change for the main package.

@AlexV525
Copy link
Member

Could work but that would also imply a breaking change for the main package.

As I imagine it won't, we can set the constraint to allow both previous and the new version

@kuhnroyal
Copy link
Member

Yea I understand now, sounds like a plan.

@kuhnroyal
Copy link
Member

We have to re-export some of the web classes in dio.

@AlexV525
Copy link
Member

Offline discussed with @huanghui1998hhh , he will go ahead to try split the package. As I & he has tested the current package does not affect the flutter build, only affects the runtime.

@AlexV525 AlexV525 marked this pull request as draft May 16, 2024 09:40
@huanghui1998hhh huanghui1998hhh marked this pull request as ready for review May 17, 2024 02:18
@huanghui1998hhh huanghui1998hhh marked this pull request as draft May 17, 2024 02:35
@@ -4,7 +4,8 @@ import 'dart:typed_data';
import 'package:meta/meta.dart';

import 'adapters/io_adapter.dart'
if (dart.library.html) 'adapters/browser_adapter.dart' as adapter;
if (dart.library.js_util) 'adapters/web_adapters/web_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

and everywhere else in the dio package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: dio Targeting `dio` package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants