-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Adapt dart2wasm #2215
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
Looks like we have to move to Dart 3.1.0 for this. |
What about we split the Web support as a package and publish it first, then make a major version upgrade for wasm? |
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 |
Yea I understand now, sounds like a plan. |
We have to re-export some of the web classes in dio. |
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 |
@@ -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' |
There was a problem hiding this comment.
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
Closing in favor of #2223 |
New Pull Request Checklist
main
branch to avoid conflicts (via merge from master or rebase)CHANGELOG.md
in the corresponding packageAdditional context and info (if any)