plus_plugins

plus_plugins copied to clipboard

Description

rename toMap() as toJson() to improve compatibility with dart:convert see jsonEncode

If toEncodable is omitted, it defaults to a function that returns the result of calling .toJson() on the unencodable object.

updated: makes toMap compatible with json parsing

Related Issues

fix https://github.com/fluttercommunity/plus_plugins/issues/949

Checklist

• [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• [x] My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
• [x] All existing and new tests are passing.
• [x] I updated the version in pubspec.yaml and CHANGELOG.md.
• [x] I updated/added relevant documentation (doc comments with ///).
• [x] The analyzer (flutter analyze) does not report any problems on my PR.
• [x] I read and followed the [Flutter Style Guide].
• [x] I am willing to follow-up on review comments in a timely manner.

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).
• [x ] No, this is not a breaking change.

TODO:

after publishing, bump device_info_plus_platform_interface to ^2.3.1 in device_info_plus's pubspec

Thank you! I am positive about this change. The only issue is that this would be a major version bump, so the platform plugin should be increased to 3.0.0, and all its dependencies as well. The change should be well documented in the respective Chanelogs.

Any thoughts? @vbuberen

Any thoughts? @vbuberen

I am not sure why do we need it.

@miquelbeltran thanks for looking up the PR

@vbuberen besides the originally stated reason, during the implementation I also noticed that in some platform toMap is "broken" more specifically jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum) this PR address this problem as well

during the implementation I also noticed that in some platform toMap is "broken" more specifically jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum) this PR address this problem as well

mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.

What do you think about that @iapicca ?

mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.

What do you think about that @iapicca ?

@miquelbeltran no problem, I re-added the toMap() straight from the current upstream main in most cases I can just toJson() => toMap(), but the web platform interface was messed up (not by me) and there is a different behavior for both toJson and fromMap please take a look

I see that a package_info_plus test fails because dependencies conflicts with flutter stable (no flutter-action?) seems outside the scope of this PR, but let me know if I need to take any action

Because package_info_plus_macos requires the Flutter SDK, version solving failed.
Running "flutter pub get" in package_info_plus_macos...         
Flutter users should run `flutter pub get` instead of `dart pub get`.
---- Log transcript ----
FINE: Pub 2.17.6
MSG : Resolving dependencies...
SLVR: fact: package_info_plus_macos is 1.3.0
SLVR: derived: package_info_plus_macos
SLVR: fact: package_info_plus_macos requires the Flutter SDK
SLVR: conflict: package_info_plus_macos requires the Flutter SDK
SLVR: Version solving took 0:00:00.080[109](https://github.com/fluttercommunity/plus_plugins/runs/7388981363?check_suite_focus=true#step:5:110) seconds.
    | Tried 1 solutions.
FINE: Resolving dependencies finished (0.1s).
ERR : Because package_info_plus_macos requires the Flutter SDK, version solving failed.
    | 
    | Flutter users should run `flutter pub get` instead of `dart pub get`.
FINE: Exception type: SolveFailure
FINE: package:pub/src/solver/version_solver.dart 311:5               VersionSolver._resolveConflict
    | package:pub/src/solver/version_solver.dart 132:27              VersionSolver._propagate
    | package:pub/src/solver/version_solver.dart 96:11               VersionSolver.solve.<fn>
    | ===== asynchronous gap ===========================
    | dart:async                                                     Future.catchError
    | package:pub/src/utils.dart 109:52                              captureErrors.wrappedCallback
    | package:stack_trace                                            Chain.capture
    | package:pub/src/utils.dart [122](https://github.com/fluttercommunity/plus_plugins/runs/7388981363?check_suite_focus=true#step:5:123):11                              captureErrors
    | package:pub/src/command.dart 183:13                            PubCommand.run
    | package:args/command_runner.dart 209:27                        CommandRunner.runCommand
    | package:pub/src/command_runner.dart 174:24                     PubCommandRunner.runCommand
    | package:pub/src/command_runner.dart 159:20                     PubCommandRunner.run
    | package:dartdev/dartdev.dart 45:56                             runDartdev
    | /b/s/w/ir/cache/builder/sdk/pkg/dartdev/bin/dartdev.dart 11:9  main
---- End log transcript ----
pub get failed (1; ---- End log transcript ----)
BootstrapException: Failed to install.: package_info_plus_macos at /home/runner/work/plus_plugins/plus_plugins/packages/package_info_plus/package_info_plus_macos.
Error: Process completed with exit code 1.

I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.

I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.

@miquelbeltran can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())

@miquelbeltran can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())

Yes, I agree with that.

The Readme.md says:

You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.

If the serialization using toMap doesn't work because it uses enums, then it must be fixed!

I did merge properly before, now it's up to date

[...] can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())

Yes, I agree with that.

The Readme.md says:

You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.

If the serialization using toMap doesn't work because it uses enums, then it must be fixed!

@miquelbeltran that's good to hear! So I removed the toJson and ported the fix for the web implementation I think this way is more than usable and if someone (like me) really wants toJson can just do

extension BaseDeviceInfoToJsonX on BaseDeviceInfo {
  Map<String, Object?> Function() get toJson => toMap;
}

Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?

Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?

All changes not related to the fix should be reverted

@miquelbeltran I think we have to option for the web plugin implementation:

a) remove browserName from both toMap and fromMap

b) keep browserName in both toMap and fromMap ensuring consistency with userAgent data

b is the current implementation and requires the committed changes, I'm not sure what changes you wish to be limited