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 web_adapter #2223

Merged
merged 16 commits into from
Jun 15, 2024
Merged

⚡️ Split web_adapter #2223

merged 16 commits into from
Jun 15, 2024

Conversation

AlexV525
Copy link
Member

@AlexV525 AlexV525 commented May 22, 2024

Breaks the web business into a separate package to prepare for further changes regarding the WASM builds.

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)

Here we put a v1 for the package. Once it works, we will migrate to the js_interop and package:web implementation and publish v2.

@CaiJingLong
Copy link
Contributor

For new plugin, publish action must be modified after the new plugin is released.
The initial release may require manual uploading to pub.dev.

@AlexV525
Copy link
Member Author

AlexV525 commented Jun 4, 2024

All greened tests can be found at https://github.com/cfug/dio/actions/runs/9360866963/job/25766895250?pr=2223

I'm going to reset it for the first publishing.

@AlexV525 AlexV525 marked this pull request as ready for review June 4, 2024 23:31
@AlexV525 AlexV525 requested a review from a team as a code owner June 4, 2024 23:31
@kuhnroyal
Copy link
Member

Coverage is working now, awesome!

@kuhnroyal
Copy link
Member

kuhnroyal commented Jun 10, 2024

Need to add the test suite test for this new adapter.

Nvm.

Copy link
Contributor

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
plugins/web_adapter/lib/src/html/adapter.dart 🔴 0% 🔴 25.27% 🟢 25.27%
Overall Coverage 🟢 81.28% 🟢 80.08% 🔴 -1.2%

Minimum allowed coverage is 0%, this run produced 80.08%

@AlexV525
Copy link
Member Author

Anyone want to stamp this? :)

Copy link
Contributor

@ueman ueman left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexV525 AlexV525 merged commit 4f4cdee into main Jun 15, 2024
3 checks passed
@AlexV525 AlexV525 deleted the feat/standalone-web branch June 15, 2024 12:22
@AlexV525
Copy link
Member Author

I think the CHANGELOG of dio should be added too, I'll commit it later

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

Choose a reason for hiding this comment

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

@AlexV525 I think we need to remove this import line everywhere. It now imports the old version specifically for WASM. I don't fully understand, why it was compiling with WASM before this was added.
After we actually add WASM support, we can add it back.

Copy link
Member Author

Choose a reason for hiding this comment

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

If this is the reason, it could be additional compile checks are applied when the library is actually getting imported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: dio Targeting `dio` package platform: web s: feature This issue indicates a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants