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

🚀 Supports the WASM environment #2274

Merged
merged 9 commits into from
Aug 13, 2024
Merged

🚀 Supports the WASM environment #2274

merged 9 commits into from
Aug 13, 2024

Conversation

AlexV525
Copy link
Member

@AlexV525 AlexV525 commented Jul 27, 2024

Resolves #2265

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

@AlexV525
Copy link
Member Author

Since we bootstrapped packages in our workflows, the minimum SDK requirement workflow reads the web_adapter from the overrides, thus hitting the dependency constraint limit. We should find a way to bypass it.

@Rexios80
Copy link

Rexios80 commented Aug 2, 2024

Why is so much effort being spent on maintaining backwards compatibility with old Dart versions? The Flutter team isn’t doing this for their packages, and I’m not aware of any other package attempting this. The most effort I’ve seen put into backwards compatibility is using a range for the web dependency which I see you already have here.

Creating a dio_web_adapter package is already causing significant development issues, and I see little benefit. This second package will add significant development overhead to landing any future fixes or features.

@AlexV525
Copy link
Member Author

AlexV525 commented Aug 8, 2024

Why is so much effort being spent on maintaining backwards compatibility with old Dart versions?

The package is widely used across different versions, dropping any SDK versions could reflected in thousands of downstream packages/projects to migrate their codebase requirements.

The Flutter team isn’t doing this for their packages

They don't, which is why the stable version could only be a major used version after a couple of releases, and they could have their policies, but that doesn't mean we need to follow them. The situations are also different in different regions, and we are not region-specific.

and I’m not aware of any other package attempting this.

Which is why people ask "why the new version is not working anymore". Previous discussion: #2176

Creating a dio_web_adapter package is already causing significant development issues and I see little benefit. This second package will add significant development overhead to landing any future fixes or features.

Could you elaborate more? What issue has it brought?

@AlexV525
Copy link
Member Author

AlexV525 commented Aug 8, 2024

@kuhnroyal Do you have any input about the min SDK check here? Once we migrate web to v2 the check becomes invalid always.

@kuhnroyal
Copy link
Member

The problem seems to be, that melos overwrites the dio_web_adapter dependency to the local version which is always the WASM compatible version. In the min SDK case we need to somehow force melos to used the published version from pub.dev and we should be fine.

@kuhnroyal
Copy link
Member

melos bootstrap seems to support all package filters (not sure if the old version compatible with Min SDK does) so we can try to pass melos bootstrap --ignore="dio_web_adapter".

Signed-off-by: Alex Li <[email protected]>
@Rexios80
Copy link

Rexios80 commented Aug 8, 2024

Alright if you're really comitted to the web_adapter approach, here are my suggestions:

Both major versions of web_adapter should co-exist in the repo. Any updates to web_adapter could affect both versions, and that will be much easier to handle if they both exist in the repo at the same time. So both of these packages should exist:

  • plugins/web_adapter/legacy (aka v1)
  • plugins/web_adapter/web (aka v2)

Of course that causes this issue with melos:

melos.yaml: Multiple packages with the name `dio_web_adapter` found in the workspace, which is unsupported.
To fix this problem, consider renaming your packages to have a unique name.

The packages that caused the problem are:
- dio_web_adapter at plugins/web_adapter/web
- dio_web_adapter at plugins/web_adapter/legacy

I personally don't use melos specifically because it causes random issues like that, so I don't have a suggested fix besides removing melos in favor of another tool (such as puby).

When both versions exist, you can add dependency overrides for the legacy and web versions of the package in CI depending on the matrix configuration and perform the relevant SDK version compatability checks.

@Rexios80
Copy link

Rexios80 commented Aug 8, 2024

Of course you could pretend that web_adapter v1 will never need updated, but that negates all the effort spent splitting it out in the first place

@Rexios80
Copy link

Rexios80 commented Aug 8, 2024

Dart 2.15 ist almost 2.5 years old, we need to get rid of it so we can move forward with the limited time we all have to spare for OS projects. If someone is still using such an old version and having actual problems due to specific bugs, then they can afford to pay someone else to solve the problem in a fork or donate to dio and request a custom handmade release.

This comment from #2176 is relevant here although Dart 3.3 is only 6 months old. Maintaining compatibility with versions prior to 3.3 significantly increases the complexity of this project with little perceived benefit.

@kuhnroyal
Copy link
Member

We can always update v1 from a different branch and deploy manually if there are critical bugs.

@Rexios80
Copy link

Rexios80 commented Aug 8, 2024

You can make whatever decision you see fit, I'm just providing insight. I don't want the maintainers of this package shooting themselves in the foot in the name of backwards compatibility. The decisions made here will affect development for the life of this package.

We can always update v1 from a different branch and deploy manually if there are critical bugs.

This is not ideal and an example of the extra overhead this approach introduces. Also it's not just bugs I'm concerned about. If a new feature needs added to both web_adapter v1 and v2, then the branching/review process becomes even more of a mess.

@Rexios80
Copy link

Rexios80 commented Aug 8, 2024

Here is my main issue with the current project setup:

By specifying the range dio_web_adapter: '>=1.0.0 <3.0.0' you are making a promise to fully support any version of dio_web_adapter in that range. Leaving web_adapter v1 to languish on some forgotten branch does not instill confidence in the upholding of that promise.

Here are some things that could affect web_adapter v1

  • Major dependency upgrades: web_adapter doesn't have many dependencies, but this could still be an issue in the future. Are you promising to keep dependencies of web_adapter v1 up to date?
  • New features: any new feature added to web_adapter v2 will need to be backported to v1
  • Breaking changes: any breaking changes to web_adapter v2 will also need to be evaluated for v1. Then you will have to make a decision on how to handle the version bump. Would v1 become v3 and v2 become v4?

@AlexV525 AlexV525 marked this pull request as ready for review August 9, 2024 04:49
@AlexV525 AlexV525 requested a review from a team as a code owner August 9, 2024 04:49
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.

What about all the commented files, can they be removed?

.github/workflows/tests.yml Show resolved Hide resolved
plugins/web_adapter/CHANGELOG.md Outdated Show resolved Hide resolved
@kuhnroyal
Copy link
Member

kuhnroyal commented Aug 9, 2024

  • Major dependency upgrades: web_adapter doesn't have many dependencies, but this could still be an issue in the future. Are you promising to keep dependencies of web_adapter v1 up to date?
  • New features: any new feature added to web_adapter v2 will need to be backported to v1
  • Breaking changes: any breaking changes to web_adapter v2 will also need to be evaluated for v1. Then you will have to make a decision on how to handle the version bump. Would v1 become v3 and v2 become v4?

Thanks for your input!

  1. I don't really see a problem regarding dependency bumps. If there is one we can manually bump and release a 1.x patch.
  2. New features is the biggest concern I have, but I am fine with only doing this for v2.
  3. Breaking changes in the adapter would likely imply breaking changes in the dio main library. At which point we will put the question of the min supported Dart SDK up for debate and can likely drop the v1 version of the adapter.

@AlexV525
Copy link
Member Author

AlexV525 commented Aug 9, 2024

What about all the commented files, can they be removed?

Originally I was keeping them to track with code changes. Once we have a v1 branch they can be removed completely IMO.

@kuhnroyal
Copy link
Member

Probably just branch it from before your commits in this PR, then it should be fine.

@AlexV525
Copy link
Member Author

AlexV525 commented Aug 9, 2024

Probably just branch it from before your commits in this PR, then it should be fine.

Yeah will do. I've just finished our local I/O Connect summit and will be back in the next week.

@AlexV525
Copy link
Member Author

v1 branch created: https://github.com/cfug/dio/tree/feat/web/v1-adapter

@AlexV525 AlexV525 requested a review from kuhnroyal August 11, 2024 09:00
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.

We have no WASM tests, we might want to add them in a separate PR for the stable/beta tracks.

@AlexV525
Copy link
Member Author

We have no WASM tests, we might want to add them in a separate PR for the stable/beta tracks.

Added the extra compiler flag to the workflow and all tests are happy so far.

Copy link
Contributor

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
Overall Coverage 🟢 82.41% 🟢 84.39% 🟢 1.98%

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

@AlexV525 AlexV525 merged commit 050b857 into main Aug 13, 2024
3 checks passed
@AlexV525 AlexV525 deleted the feat/wasm-web branch August 13, 2024 09:41
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.

Is it now possible to use DIO / dio_web_adapter with Web/WASM?
3 participants