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

Migrated to package:web #1087

Open
wants to merge 49 commits into
base: minor
Choose a base branch
from
Open

Migrated to package:web #1087

wants to merge 49 commits into from

Conversation

ali2236
Copy link

@ali2236 ali2236 commented Aug 20, 2024

Migrated audio_service_web to use package:web instead of dart:html, dart:js & dart:js_util.

I also updated the example app and tested with it to see if the new web changes were working.

Pre-launch Checklist

  • I read the [CONTRIBUTING.md] and followed the process outlined there for submitting PRs.
  • My change is not breaking and lands in minor branch OR my change is breaking and lands in major branch.
  • If I'm the first to contribute to the next version, I incremented the version number in pubspec.yaml according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change (format: * DESCRIPTION OF YOUR CHANGE (@your-git-username)).
  • I updated/added relevant documentation (doc comments with ///).
  • I ran dart analyze.
  • I ran dart format.
  • I ran flutter test and all tests are passing.

@ryanheise
Copy link
Owner

Is this a breaking change? (Since it is to be merged into major)
It doesn't seem like this needs to be breaking.

@ali2236
Copy link
Author

ali2236 commented Dec 15, 2024

Is this a breaking change? (Since it is to be merged into major) It doesn't seem like this needs to be breaking.

It's been quite a while, but there are no library API changes. only changes to how web APIs are called. I only tested it with the example project and it worked without any modifications.

@ryanheise
Copy link
Owner

That's as expected, so I think this could be rebased to minor for a non-breaking release.

@mtallenca
Copy link

Any ETA on this PR?

@ali2236 ali2236 changed the base branch from major to minor January 15, 2025 19:30
@ali2236
Copy link
Author

ali2236 commented Jan 15, 2025

@mtallenca I'll see what I can do.

@mtallenca
Copy link

Thanks @ali2236 ! @ryanheise Anything else needs to be done for this PR?

@ryanheise
Copy link
Owner

Sorry, I've just been in hospital and recovering so I'm not currently in a good condition to review/merge just yet, but let's give it 2-3 days and see how I'm feeling.

@mtallenca
Copy link

@ryanheise Sorry to hear you are in hospital recovering. Hope you get better soon. Please let me know if there is anything I can help out with regarding the repo.

@drumkiller-peter
Copy link

drumkiller-peter commented Jan 21, 2025

@ali2236 Following up, will this be supporting WASM??

@ali2236
Copy link
Author

ali2236 commented Jan 21, 2025

@ali2236 Following up, will this be supporting WASM??

Yes. this removes the old js dependency and replaces it with the new wasm compatible web package

@ryanheise
Copy link
Owner

I'm starting to feel somewhat better. I'm going to sleep now but will take a look at this the next day.

@ryanheise
Copy link
Owner

Unfortunately this is going to be rather complicated to merge. A rebase would have been nice, but I think a merge has been done instead, hence it saying there are now 49 commits to be merged into minor. This also includes part of the major branch which should not be included in minor. I've tried cherry picking the relevant commits to reconstruct the PR manually, but that seems to be also missing some important changes, so I suspect that some other changes were also done inside the merge commit. I'm going to take a break and come back to this with fresh eyes later...

@mtallenca
Copy link

Glad you are feeling better. Let me know if there is anything I could do to help out.

@ryanheise
Copy link
Owner

I've put my changes in a new PR here: #1102 if you'd like to test. It cherry picks the key commits from this PR, updates versions, fixes nullability and formatting.

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