refactor(battery_plus)!: platform implementation refactor into a single package #1169

miquelbeltran · 2022-10-05T19:30:14Z

Description

After multiple discussions of the maintainer team, we came to the conclusion that maintaining a federated plugin architecture was more painful than brought any advantages.

This PR includes the work to "unfederate" battery_plus.

I decided to pick this package since there are no open PRs targeting this plugin.

Note to reviewers: The PR is easier to see if you checkout the branch.

Work Done

To perform the "unfederation":

I moved all code from the respective plugins into the main battery_plus plugin.
Then, I renamed all references to the external plugins to the main one, e.g. battery_plus_linux became battery_plus.
Also, updated the references to the paths to be local to the package.
Some unit tests that existed on the platform packages were moved.
The package_interface package also suffered the same fate as the other platform plugins and the code was moved to the main plugin. The platform interface still exists and could theoretically be used by an external plugin that implements a new platform (e.g. Fuchsia?)

Update:

battery_plus_package_interface will remain as independent package.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

miquelbeltran · 2022-10-05T19:55:19Z

Windows build is not working, will try to fix tomorrow.

jpnurmi

I'm curious, is there a technical reason to have battery_plus_web.dart in lib/ instead of lib/src/?

packages/battery_plus/lib/battery_plus.dart

nohli · 2022-10-06T05:38:15Z

Yayy the tests are passing 😊

miquelbeltran · 2022-10-06T05:44:23Z

I'm curious, is there a technical reason to have battery_plus_web.dart in lib/ instead of lib/src/?

This has been addressed ;)

miquelbeltran · 2022-10-06T05:46:28Z

Yayy the tests are passing blush

I am happy that I didn't need to do any CI changes! Only to make sure that the projects compiled.

I had to switch to my Windows PC to fix the Windows build but that was it.

nohli · 2022-10-06T06:11:01Z

The six folders can be deleted, right?
Edit: Nevermind, was only because they contained files not checked into git. Is there maybe a way to delete these folders in local clones although they contain untracked files?

Why is there no linux folder and is pluginClass: none correct?

miquelbeltran · 2022-10-06T06:16:55Z

The six folders can be deleted, right? Edit: Nevermind, was only because they contained files not checked into git. Is there maybe a way to delete these folders in local clones although they contain untracked files?

The folders will still exist on your local copy, a git clean -dxf will take care of that for you.

Why is there no linux folder and is pluginClass: none correct?

I believe so, since the Linux implementation is purely in Dart. The pluginClass: none existed in the battery_plus_linux pubspec so I copied it over. I am not 100% sure if it is required or not.

jpnurmi · 2022-10-06T06:20:50Z

I believe so, since the Linux implementation is purely in Dart. The pluginClass: none existed in the battery_plus_linux pubspec so I copied it over. I am not 100% sure if it is required or not.

It was a temporary workaround that is probably not needed anymore: flutter/flutter#57497

nohli · 2022-10-06T06:35:22Z

How about moving everything into src folder?
And perhaps renaming method_channel_battery_plus.dart to battery_plus_method_channel.dart?

Edit: Do we need the platform interface in the lib folder (not in src)?

jpnurmi · 2022-10-06T06:35:39Z

~~One more, is it possible to move method_channel_battery_plus.dart into src/ as well so that we'd only have battery_plus.dart and the platform interface in the root?~~

EDIT: What @nohli just said above :)

jpnurmi · 2022-10-06T07:26:38Z

Very nice! 👍 LGTM but we need Majid's blessing for this...

miquelbeltran · 2022-10-06T07:27:03Z

Sorry for going back and forth but now that I think of it, having a separate battery_plus_platform_interface.dart at the top level (instead of exporting all of the platform interface) would avoid polluting the usual import with BatteryPlatform.

Right. Also, we can now export the enums.dart directly instead of going through the platform_interface file.

packages/battery_plus/pubspec.yaml

nohli · 2022-10-06T08:08:39Z

LGTM! ❤️

Also letting @mhadaily give the approval 🙂

miquelbeltran · 2022-10-06T15:08:22Z

packages/battery_plus/battery_plus/pubspec.yaml


 dependencies:
  flutter:
    sdk: flutter
+  flutter_web_plugins:
+    sdk: flutter
+  battery_plus_platform_interface: 1.2.1


as decided, the platform_interface is going to be referenced by exact version, not by "^"

nohli · 2022-10-06T15:08:30Z

packages/battery_plus/battery_plus/lib/battery_plus.dart

@@ -37,7 +39,7 @@ class Battery {

  /// check if device is on battery save mode
  ///
-  /// Currently only impemented on Android, IOS and Windows.
+  /// Currently only implemented on Android, IOS and Windows.


Since we're fixing typos, iOS would be better

I couldn't let it go, been seeing the typo the whole day 😂

That's the spirit!

miquelbeltran · 2022-10-06T15:28:52Z

I wonder if we should modify this CI check:

 Downloading battery_plus_platform_interface 1.2.1...
Package validation found the following potential issue:
* Your dependency on "battery_plus_platform_interface" should allow more than one version. For example:
  
  dependencies:
    battery_plus_platform_interface: ^1.2.1
  
  Constraints that are too tight will make it difficult for people to use your package
  along with other packages that also depend on "battery_plus_platform_interface".

vbuberen

Looks good to me

miquelbeltran added 6 commits October 5, 2022 21:05

unfederate battery_plus_linux

e40d37c

unfederate battery_plus_macos

d41abcc

unfederate battery_plus_web

5fd724f

unfederate battery_plus_windows

76604bc

Unfederate the platform interface

1357c3a

move all files one dir below

b8a94a9

miquelbeltran marked this pull request as draft October 5, 2022 19:33

miquelbeltran added 4 commits October 5, 2022 21:35

fix dependency

8739d55

bump version

ffc60c7

add missing flutter_web_plugins

281a2d6

windows naming fix

20b02b7

miquelbeltran requested review from jpnurmi, mhadaily, nohli and vbuberen October 5, 2022 20:00

jpnurmi reviewed Oct 5, 2022

View reviewed changes

packages/battery_plus/lib/battery_plus.dart Outdated Show resolved Hide resolved

Fix windows build

9ca606a

miquelbeltran marked this pull request as ready for review October 6, 2022 05:36

Move battery_plus_web.dart to src folder

27e5143

remove pluginClass: none

fee455a

moving all files to src

868a15a

miquelbeltran added 3 commits October 6, 2022 09:20

sort imports

47cc155

move battery_plus_platform_interface.dart to root

0f1dd77

export enums directly instead of the battery_plus_platform_interface

d622afd

nohli reviewed Oct 6, 2022

View reviewed changes

packages/battery_plus/pubspec.yaml Outdated Show resolved Hide resolved

rename BatteryPlusWebPlugin

66ef252

nohli reviewed Oct 6, 2022

View reviewed changes

packages/battery_plus/pubspec.yaml Outdated Show resolved Hide resolved

renamed BatteryPlusLinux to BatteryPlusLinuxPlugin for consistency

92e12f9

miquelbeltran added 3 commits October 6, 2022 16:58

move battery_plus into battery_plus

d695aaf

re-add battery_plus_platform_interface

5139ab6

fix battery_plus dependency to the platform interface

5d9e504

miquelbeltran commented Oct 6, 2022

View reviewed changes

nohli reviewed Oct 6, 2022

View reviewed changes

typo

0a18675

miquelbeltran changed the title ~~Unfederate battery_plus~~ BREAKING CHANGE: battery_plus platform implementations refactor Oct 6, 2022

miquelbeltran added 2 commits October 6, 2022 17:16

format

1a69268

remove new line from changelog, let's try melos release process

6c51388

miquelbeltran changed the title ~~BREAKING CHANGE: battery_plus platform implementations refactor~~ BREAKING CHANGE(battery_plus): platform implementations refactor into a single package Oct 6, 2022

miquelbeltran changed the title ~~BREAKING CHANGE(battery_plus): platform implementations refactor into a single package~~ BREAKING CHANGE(battery_plus): platform implementation refactor into a single package Oct 6, 2022

revert forcing package version

3395fcd

miquelbeltran requested review from jpnurmi and nohli October 6, 2022 15:45

miquelbeltran changed the title ~~BREAKING CHANGE(battery_plus): platform implementation refactor into a single package~~ refactor(battery_plus)!: platform implementation refactor into a single package Oct 6, 2022

vbuberen approved these changes Oct 8, 2022

View reviewed changes

miquelbeltran merged commit 143204b into main Oct 8, 2022

miquelbeltran deleted the battery_plus_unfederate branch October 8, 2022 14:56

miquelbeltran added the hacktoberfest-accepted label Oct 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(battery_plus)!: platform implementation refactor into a single package #1169

refactor(battery_plus)!: platform implementation refactor into a single package #1169

miquelbeltran commented Oct 5, 2022 •

edited

Loading

miquelbeltran commented Oct 5, 2022

jpnurmi left a comment

nohli commented Oct 6, 2022

miquelbeltran commented Oct 6, 2022

miquelbeltran commented Oct 6, 2022

nohli commented Oct 6, 2022 •

edited

Loading

miquelbeltran commented Oct 6, 2022 •

edited

Loading

jpnurmi commented Oct 6, 2022

nohli commented Oct 6, 2022 •

edited

Loading

jpnurmi commented Oct 6, 2022 •

edited

Loading

jpnurmi commented Oct 6, 2022

miquelbeltran commented Oct 6, 2022

nohli commented Oct 6, 2022

miquelbeltran Oct 6, 2022

nohli Oct 6, 2022

miquelbeltran Oct 6, 2022

nohli Oct 6, 2022

miquelbeltran commented Oct 6, 2022

vbuberen left a comment

refactor(battery_plus)!: platform implementation refactor into a single package #1169

refactor(battery_plus)!: platform implementation refactor into a single package #1169

Conversation

miquelbeltran commented Oct 5, 2022 • edited Loading

Description

Work Done

Breaking Change

miquelbeltran commented Oct 5, 2022

jpnurmi left a comment

Choose a reason for hiding this comment

nohli commented Oct 6, 2022

miquelbeltran commented Oct 6, 2022

miquelbeltran commented Oct 6, 2022

nohli commented Oct 6, 2022 • edited Loading

miquelbeltran commented Oct 6, 2022 • edited Loading

jpnurmi commented Oct 6, 2022

nohli commented Oct 6, 2022 • edited Loading

jpnurmi commented Oct 6, 2022 • edited Loading

jpnurmi commented Oct 6, 2022

miquelbeltran commented Oct 6, 2022

nohli commented Oct 6, 2022

miquelbeltran Oct 6, 2022

Choose a reason for hiding this comment

nohli Oct 6, 2022

Choose a reason for hiding this comment

miquelbeltran Oct 6, 2022

Choose a reason for hiding this comment

nohli Oct 6, 2022

Choose a reason for hiding this comment

miquelbeltran commented Oct 6, 2022

vbuberen left a comment

Choose a reason for hiding this comment

miquelbeltran commented Oct 5, 2022 •

edited

Loading

nohli commented Oct 6, 2022 •

edited

Loading

miquelbeltran commented Oct 6, 2022 •

edited

Loading

nohli commented Oct 6, 2022 •

edited

Loading

jpnurmi commented Oct 6, 2022 •

edited

Loading