Code refactoring + more safety with freezed #270

devasidmi · 2021-11-05T13:42:50Z

Code improvements + more safety with hashcodes & json parsing with Freezed package
No PR or issue number

Single test failed because of wrong API key

aboedo · 2021-11-10T14:25:42Z

Hi!! 👋 Thanks for opening and sorry about the delay in replying!

This PR is absolutely amazing 🏆

Love how much cleaner this looks. Will do a more thorough review now, just wanted to start by stating how big a fan I am of having the serialization done in models instead of manually.

aboedo

this PR is AWESOME 🔥. Thanks so much for opening!
I'm still going through it, sorry if I'm going slow.
I left a few comments in the meantime, let me know what you think!

analysis_options.yaml

aboedo · 2021-11-12T18:01:37Z

analysis_options.yaml

+    unnecessary_string_interpolations: true
+    unnecessary_this: true
+    invalid_annotation_target: false


love the new rules

nitpick: could we keep these alphabetized? I think this is the only one that isn't

fixed that one

analysis_options.yaml

lib/errors.dart

aboedo · 2021-11-12T20:30:46Z

lib/models/offerings_wrapper.dart

+    @JsonKey(name: 'current', nullable: true) Offering? current,
+  }) = _Offerings;
+
+  factory Offerings.fromJson(Map<String, dynamic> json) =>


this class looks much better than before, but the one missing element from it is the ability to get an offering by name, could you add it?

checked for other getters|methods, my bad - fixed

aboedo · 2021-11-12T20:48:33Z

lib/models/offerings_wrapper.dart

+
+/// This class contains all the offerings configured in RevenueCat dashboard.
+/// For more info see https://docs.revenuecat.com/docs/entitlements
+class Offerings with _$Offerings {


I think there might be an issue with the mapping for this class, when testing using the MagicWeather app I'm running into type '_InternalLinkedHashMap<Object?, Object?>' is not a subtype of type 'Map<String, dynamic>' when trying to getOfferings. Does it work correctly for you? Maybe I'm missing a step

Great catch, below are the fixes

aboedo · 2021-11-12T20:51:33Z

lib/models/offering_wrapper.dart

+    final String serverDescription,
+
+    /// Array of `Package` objects available for purchase.
+    final List<Package> availablePackages, {


perhaps it would be clearer for readers to have the { either as a separate line or as the first line in the first optional parameter, to clearly delineate the beginning of optional params? I had missed it at first, being at the end of this line

any thoughts on this?

Thoughts on this? @devasidmi

It's vscode autoformatting, do u have any idea how to change that behaviour without changing editor settings?

I don't know 🤷‍♂️ . But if that's how it's auto-formatted, let's leave it like that

lib/purchases_flutter.dart

devasidmi · 2021-11-14T13:19:43Z

this PR is AWESOME 🔥. Thanks so much for opening! I'm still going through it, sorry if I'm going slow. I left a few comments in the meantime, let me know what you think!

Thank you, I really wanted to participate and make the best subscription package even better ✌🏻

lib/models/introductory_price.dart

aboedo · 2021-11-15T17:23:20Z

lib/models/offering_wrapper.dart

+    final String serverDescription,
+
+    /// Array of `Package` objects available for purchase.
+    final List<Package> availablePackages, {


any thoughts on this?

aboedo · 2021-11-15T17:57:37Z

lib/models/product_wrapper.dart

+    @JsonKey(name: 'currency_code') String currencyCode, {
+
+    /// Introductory price for product. Can be null.
+    @JsonKey(name: 'introPrice', nullable: true)


I believe this needs to be intro_price https://github.com/RevenueCat/purchases-flutter/blob/main/lib/product_wrapper.dart#L35

aboedo · 2021-11-15T17:59:56Z

lib/models/introductory_price.dart

+class IntroductoryPrice with _$IntroductoryPrice {
+  const factory IntroductoryPrice(
+    /// Introductory price of a subscription in the local currency.
+    @JsonKey(name: 'price') double introPrice,


this is intro_price https://github.com/RevenueCat/purchases-flutter/blob/main/lib/product_wrapper.dart#L78

Could you help me with this test pls? I think that response is not valid because of introPrice is Map<String, dynamic> not int

The response is fine. The issue is that the JsonKey to use in product_wrapper for the introductoryPrice should be introPrice instead of intro_price. introductory_price.dart also needs to be updated to match the keys in the introPrice object of the response:

'introPrice': { 'price': 0, 'priceString': '\$0.00', 'period': 'P1W', 'cycles': 1, 'periodUnit': 'DAY', 'periodNumberOfUnits': 7 },

The information is duplicated in the response, because we used to have the intro price information in the root of the JSON, then we moved it to its own object introPrice and changed the json keys to be camelcase. We left the keys in the JSON root because other SDKs still use those :)

Let me know if you want me to do the update to fix the test :)

aboedo · 2021-11-15T18:00:32Z

lib/models/introductory_price.dart

+
+    /// Formatted introductory price of a subscription, including
+    /// its currency sign, such as €3.99.
+    @JsonKey(name: 'priceString') String introPriceString,


the mappings here should start with intro_ and use snake_case, right?

lib/purchases_flutter.dart

aboedo · 2021-11-15T18:40:51Z

lib/purchases_flutter.dart

+    final purchaserInfo = PurchaserInfo.fromJson(
+      Map<String, dynamic>.from(result['purchaserInfo']),
+    );
+    final bool created = result['created'];


[not needed for this PR] we could add a loginResult.dart freezed model to map this

aboedo · 2021-11-15T19:07:46Z

lib/purchases_flutter.dart

-  static Future<void> close() async {
-    await _channel.invokeMethod('close');
-  }
+  static Future<void> close() => _channel.invokeMethod('close');


do these not need async when using =>?

not really, because _channel.invokeMethod returns Future<T> and we don't need await it. Only caller of method close can await it (but it's optional).

aboedo · 2021-11-15T19:08:12Z

Thanks so much for updating! I left a few more comments

devasidmi · 2021-11-25T13:06:57Z

I believe I got u right @vegaro

vegaro · 2021-12-02T01:52:42Z

@devasidmi sorry I missed the review request. I approved it but now there's some conflicts because we added a new property to entitlement info 😮‍💨

aboedo · 2021-12-06T14:13:58Z

@devasidmi sorry to ask any more of you, but would you mind rebasing or otherwise solving the merge conflicts so we can merge?

devasidmi · 2021-12-08T18:59:01Z

@devasidmi sorry to ask any more of you, but would you mind rebasing or otherwise solving the merge conflicts so we can merge?

Sorry for the delays in the responses, I will do it soon

…es-flutter

devasidmi · 2021-12-12T09:36:45Z

@vegaro @aboedo

vegaro

Thanks so much for this PR. And sorry it took so long to merge 🥴

Vasily Dmitriev added 4 commits November 5, 2021 12:27

clean up code with lint rules

44ee7d6

freezed models -> more safe with hashcodes and json parsing

edf6386

codegen without fvm

1d459d7

tiny test fix

d6b22be

aboedo self-assigned this Nov 10, 2021

aboedo reviewed Nov 12, 2021

View reviewed changes

devasidmi commented Nov 14, 2021

View reviewed changes

lib/purchases_flutter.dart Show resolved Hide resolved

fixed pr comments

cae7581

devasidmi force-pushed the refactoring/purchases-flutter branch from d6ab05d to cae7581 Compare November 14, 2021 13:14

devasidmi requested a review from aboedo November 14, 2021 13:21

aboedo reviewed Nov 15, 2021

View reviewed changes

models fixes

0b6720c

devasidmi requested a review from aboedo November 19, 2021 14:08

tests & models fixes

79b7b0e

devasidmi requested a review from vegaro November 25, 2021 13:07

vegaro approved these changes Dec 1, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into refactoring/purchas…

bcdc24e

…es-flutter

vegaro approved these changes Dec 12, 2021

View reviewed changes

vegaro merged commit b785423 into RevenueCat:main Dec 12, 2021

vegaro mentioned this pull request Jan 18, 2022

_InternalLinkedHashMap<Object?, Object?>' is not a subtype of type 'Map<String, dynamic> error while purchasePackage() called #300

Closed

5 tasks

joshdholtz mentioned this pull request Feb 11, 2022

Breaking change not treated as breaking change #318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code refactoring + more safety with freezed #270

Code refactoring + more safety with freezed #270

devasidmi commented Nov 5, 2021

aboedo commented Nov 10, 2021

aboedo left a comment

aboedo Nov 12, 2021

aboedo Nov 12, 2021

devasidmi Nov 14, 2021

aboedo Nov 12, 2021

devasidmi Nov 14, 2021 •

edited

aboedo Nov 12, 2021

devasidmi Nov 14, 2021

aboedo Nov 12, 2021

aboedo Nov 15, 2021

vegaro Nov 26, 2021

devasidmi Nov 30, 2021

vegaro Dec 2, 2021

devasidmi commented Nov 14, 2021

aboedo Nov 15, 2021

aboedo Nov 15, 2021

aboedo Nov 15, 2021

devasidmi Nov 17, 2021 •

edited

vegaro Nov 24, 2021

aboedo Nov 15, 2021

aboedo Nov 15, 2021

aboedo Nov 15, 2021

devasidmi Nov 17, 2021 •

edited

aboedo commented Nov 15, 2021

devasidmi commented Nov 25, 2021 •

edited

vegaro commented Dec 2, 2021 •

edited

aboedo commented Dec 6, 2021

devasidmi commented Dec 8, 2021

devasidmi commented Dec 12, 2021 •

edited

vegaro left a comment

Code refactoring + more safety with freezed #270

Code refactoring + more safety with freezed #270

Conversation

devasidmi commented Nov 5, 2021

aboedo commented Nov 10, 2021

aboedo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devasidmi Nov 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devasidmi commented Nov 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devasidmi Nov 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devasidmi Nov 17, 2021 • edited

Choose a reason for hiding this comment

aboedo commented Nov 15, 2021

devasidmi commented Nov 25, 2021 • edited

vegaro commented Dec 2, 2021 • edited

aboedo commented Dec 6, 2021

devasidmi commented Dec 8, 2021

devasidmi commented Dec 12, 2021 • edited

vegaro left a comment

Choose a reason for hiding this comment

devasidmi Nov 14, 2021 •

edited

devasidmi Nov 17, 2021 •

edited

devasidmi Nov 17, 2021 •

edited

devasidmi commented Nov 25, 2021 •

edited

vegaro commented Dec 2, 2021 •

edited

devasidmi commented Dec 12, 2021 •

edited