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

chore(deps): bump web from 0.5.1 to 1.0.0 in /packages/battery_plus/battery_plus #3103

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Jul 22, 2024

Bumps web from 0.5.1 to 1.0.0.

Release notes

Sourced from web's releases.

package:web v1.0.0

  • Added supertypes' fields to dictionary constructors as dictionaries are allowed to contain those fields.
  • Empty dictionary constructors now create an empty object instead of being treated like non-object literal external constructors.
  • Generate MDN API documentation for getters.
  • Update the docs for generated interface types to reference the MDN Web Docs project.
  • Address several broken links in API documentation.
  • Generate APIs based on if they're on track to be a standard and not experimental. This is a looser restriction from the previous requirement that APIs need to be implemented by Chrome, Firefox, and Safari. As part of this, dictionaries and typedefs are only emitted if they're used by a generated API.
  • Added onUnload event stream to ElementEventGetters extension methods.
  • Expose ElementStream as a public class.
  • Require Dart ^3.4.0.
  • APIs that return a double value now return double instead of num. This is to avoid users accidentally downcasting num, which has different semantics depending on whether you compile to JS or Wasm. See issue #57[] for more details.
  • Fix an issue where some union types didn't account for typedef nullability. #57: dart-lang/web#57
Changelog

Sourced from web's changelog.

1.0.0

  • Added supertypes' fields to dictionary constructors as dictionaries are allowed to contain those fields.
  • Empty dictionary constructors now create an empty object instead of being treated like non-object literal external constructors.
  • Generate MDN API documentation for getters.
  • Update the docs for generated interface types to reference the MDN Web Docs project.
  • Address several broken links in API documentation.
  • Generate APIs based on if they're on track to be a standard and not experimental. This is a looser restriction from the previous requirement that APIs need to be implemented by Chrome, Firefox, and Safari. As part of this, dictionaries and typedefs are only emitted if they're used by a generated API.
  • Added onUnload event stream to ElementEventGetters extension methods.
  • Expose ElementStream as a public class.
  • Require Dart ^3.4.0.
  • APIs that return a double value now return double instead of num. This is to avoid users accidentally downcasting num, which has different semantics depending on whether you compile to JS or Wasm. See issue #57[] for more details.
  • Fix an issue where some union types didn't account for typedef nullability.

#57: dart-lang/web#57

Commits

Dependabot compatibility score

You can trigger a rebase of this PR by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

@dependabot dependabot bot added dart Pull requests that update Dart code dependencies Pull requests that update a dependency file labels Jul 22, 2024
@miquelbeltran
Copy link
Member

Closes #3093

@vbuberen
Copy link
Collaborator

I am having issues with compilation while trying to test this PR. Getting same errors as our test workflow.

@Rexios80
Copy link
Contributor

Don't forget to remove this extension if it is no longer necessary:

extension type BatteryManager(JSObject _) implements JSObject {

@vbuberen
Copy link
Collaborator

Thanks.I am aware of it

Bumps [web](https://github.com/dart-lang/web) from 0.5.1 to 1.0.0.
- [Release notes](https://github.com/dart-lang/web/releases)
- [Changelog](https://github.com/dart-lang/web/blob/main/CHANGELOG.md)
- [Commits](dart-lang/web@v0.5.1...v1.0.0)

---
updated-dependencies:
- dependency-name: web
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
@vbuberen vbuberen force-pushed the dependabot/pub/packages/battery_plus/battery_plus/web-1.0.0 branch from 9e82119 to 5eb2063 Compare July 29, 2024 10:48
@koji-1009
Copy link
Contributor

dart-lang/web#239

getBatteryManager() is available in web: ^1.0.0. So if we write web:">=0.5.0 <2.0.0" in battery_plus's pubspec.yaml, the build will fail.

There are two options.
One is to request web: ^1.0.0 with battery_plus. The other is to give the getBatteryManager method an alias. The idea is as in the following snippet.

class BatteryPlusWebPlugin extends BatteryPlatform {

~~~

  /// Return [BatteryManager] if the BatteryManager API is supported by the User Agent.
  Future<BatteryManagerPlus?> _getBatteryManager() async {
    try {
      return await web.window.navigator.getBatteryPlus()?.toDart;
    } on NoSuchMethodError catch (_) {
      // BatteryManager API is not supported this User Agent.
      return null;
    } on Object catch (_) {
      // Unexpected exception occurred.
      return null;
    }
  }

~~~

}

extension on web.Navigator {
  /// https://developer.mozilla.org/en-US/docs/Web/API/Navigator/getBattery
  @JS('getBattery')
  external JSPromise<BatteryManagerPlus>? getBatteryPlus();
}

/// BatteryManager API
/// https://developer.mozilla.org/en-US/docs/Web/API/BatteryManager
extension type BatteryManagerPlus(JSObject _) implements JSObject {
  /// https://developer.mozilla.org/en-US/docs/Web/API/BatteryManager/level
  external double get level;

  /// https://developer.mozilla.org/en-US/docs/Web/API/BatteryManager/charging
  external bool get charging;

  /// https://developer.mozilla.org/en-US/docs/Web/API/BatteryManager/chargingchange_event
  external set onchargingchange(JSFunction fn);
}

@Rexios80
Copy link
Contributor

Are you sure the build fails? I'm pretty sure the extension will just have no effect.

@Rexios80
Copy link
Contributor

The current implementation can't build because of having two different classes with the name BatteryManager not because of having conflicting getBattery() methods

@koji-1009
Copy link
Contributor

The getBattery() method and the BatteryManager class were added in v1.0.0. In v0.5.1, this method and class do not exist because they do not support all browsers.

https://github.com/dart-lang/web/blob/v0.5.1/lib/src/dom/html.dart
https://github.com/dart-lang/web/tree/v0.5.1/lib/src/dom

https://github.com/dart-lang/web/blob/v1.0.0/lib/src/dom/html.dart#L11670
https://github.com/dart-lang/web/blob/v1.0.0/lib/src/dom/battery_status.dart

Therefore, when you update package:web to v1.0.0, the getBattery method will collide first. And the return value of the getBattery method will be web.BatteryManager.
However, the current implementation (to support v0.5.1) defines its own BatteryManager as web.window.navigator.getBattery()?.toDart returned type. This causes a BatteryManager type conflict.

I believe we need our own getBatteryPlus and BatteryManagerPlus definitions to support v0.5.1. If we only support v1.0.0 and above, then there is no need to implement the extention type.

@vbuberen
Copy link
Collaborator

Therefore, when you update package:web to v1.0.0, the getBattery method will collide first. And the return value of the getBattery method will be web.BatteryManager.
However, the current implementation (to support v0.5.1) defines its own BatteryManager as web.window.navigator.getBattery()?.toDart returned type. This causes a BatteryManager type conflict.

Indeed, this is how it currently is, similar to what happens in share_plus (where I already got rid of extension).

I believe we need our own getBatteryPlus and BatteryManagerPlus definitions to support v0.5.1. If we only support v1.0.0 and above, then there is no need to implement extensions.

This is not a requirement to have the range. Good to have, but only if there is no situations like in this package with need to have extension before 1.0.0 and no need afterwards.

@miquelbeltran miquelbeltran self-requested a review July 30, 2024 08:26
@koji-1009
Copy link
Contributor

koji-1009 commented Aug 7, 2024

Sorry, I mistakenly thought that the maintainer wanted to specify the same range for battery_plus (if possible), since the other packages were described as ">=0.5.0 <2.0.0".

@miquelbeltran miquelbeltran self-assigned this Aug 7, 2024
@miquelbeltran
Copy link
Member

Tested on chrome, desktop computer, got 100% battery. Seems good?

@miquelbeltran miquelbeltran requested a review from vbuberen August 7, 2024 14:51
@miquelbeltran
Copy link
Member

@Rexios80 and @koji-1009 if you can, please take a look and let me know if it looks correct.

@miquelbeltran miquelbeltran removed their request for review August 7, 2024 14:54
@@ -34,7 +34,7 @@ dependencies:
battery_plus_platform_interface: ^2.0.1
meta: ^1.8.0
upower: ^0.7.0
web: ^0.5.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we doing the range ">=0.5.1 <2.0.0" like the packages repo did for these?

flutter/packages#7202

Copy link
Member

Choose a reason for hiding this comment

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

In this case, switching to ^1.0.0 is preferable since we want to use the newly available APIs changes in the web package, i.e. getBattery()

Copy link
Contributor

Choose a reason for hiding this comment

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

We kept the custom extensions in those packages in order to support both 0.5.0 and 1.0.0

Copy link
Member

Choose a reason for hiding this comment

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

🤔 I thought we agreed to remove the custom extensions since 1.0.0 already provide those types.

Personally, I am more keen to use 1.0.0 directly and remove the extensions, and only keep the range in cases where no extensions are involved (e.g. with other plugins like device_info_plus).

Copy link
Collaborator

Choose a reason for hiding this comment

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

My vote is for going ^1.0.0 here and in the share_plus.

@koji-1009
Copy link
Contributor

I have verified that it works as expected with Firefox and Safari on macOS. These browsers do not have the Battery API, so a NoSuchMethodError is returned when calling window.navigator.getBattery().toDart.
Since it is handled by try-on-catch, I think no-problem. Thanks!

When I try wasm build + Chrome, I get a TypeError. This is also happening on the main branch, so I'll create a separate PR.

Copy link
Collaborator

@vbuberen vbuberen left a comment

Choose a reason for hiding this comment

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

Looks good to me

@miquelbeltran miquelbeltran merged commit cf534ee into main Aug 9, 2024
20 checks passed
@miquelbeltran miquelbeltran deleted the dependabot/pub/packages/battery_plus/battery_plus/web-1.0.0 branch August 9, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dart Pull requests that update Dart code dependencies Pull requests that update a dependency file PR: needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants