SwiftTwitch icon indicating copy to clipboard operation
SwiftTwitch copied to clipboard

Remove Marshal dependency

Open kevinrpb opened this issue 4 years ago • 6 comments

Marshal is a nice utility, but in the end the built-in Decodable protocol offers the same functionality. The performance gain Marshal is said to offer is marginal for this project, IMO, given that it mostly contains structs with Int and String values in them. Instead, it adds the drag of having to maintain a dependency (plus it has not been updated in almost a year...) and a lot of boilerplate code.

So in this PR (which is still a draft becase it needs some work and it relies on #5) I will:

  • [X] Make all previously Unmarshaling type conform to Codable
  • [X] Remove init(object: MarshaledObject) initialisers and conformance to Unmarshaling
  • [X] Remove conformances to Marshal's ValueType
  • [X] Update places where deserialisation takes place to use JSONDecoder
  • [X] Update places where types are converted into Dictionaries to use JSONEncoder instead of using JSONSerialization
  • [ ] Create new structs to use in place of dictionaries when setting query/body parameters of URLRequest
    • [X] Analytics
      • [X] GetExtensionAnalyticsParams
      • [X] GetGameAnalyticsParams
    • [X] Bits
      • [X] GetBitsLeaderboardParams
    • [X] Clips
      • [X] CreateClipParams
      • [X] GetClipsParams
    • [X] Games
      • [X] GetTopGamesParams
      • [X] GetGamesParams
    • [ ] Streams
      • [X] GetStreamsParams
      • [ ] GetFollowedStreamsParams
      • [X] GetStreamsMetadataParams
      • [X] CreateStreamMarkerParams
      • [X] GetStreamMarkersParams
    • [X] Users
      • [X] GetUsersParams
      • [X] GetUsersFollowsParams
      • [X] UpdateUserParams
    • [X] Videos
      • [X] GetVideosParams
  • [ ] Update existing Result/Data structs to fix Codable conformance problems (var names vs. web keys, nested data....)
    • [ ] Analytics
    • [ ] Bits
    • [ ] Clips
    • [ ] Games
    • [ ] Streams
    • [ ] Users
    • [ ] Videos
  • [X] Update performAPIWebRequest method to make use of Codable objects rather than dictionaries
  • [ ] Add/Update documentation comments

Input on this is appreciated. Please let me know your thoughts.


This PR breaks source compatibility

In order to support Codable and make the API more Swifty (added type safety) by using custom structs instead of dictionaries I made the following changes:

  • Public API calls now receive an struct containing the parameters of the request (if appropriate) instead of the raw values (This one breaks compatibility)
  • Private API method performAPIWebRequest now uses 3 generic parameters:
    • The result from the call (was already being used)
    • The query parameters (new)
    • The body parameters (new)
  • WebRequestKeys are no longer. By using JSONEncoderand JSONDecoder it is possible to specify the keyEncodingStrategy. Therefore, as long as we keep the variable names as they should be in the API, we shouldn't need to store the keys. As a fallback in case there are strange keys we would like to modify, we could use custom CodingKeys. I moved the existing keys struct to its own file to maintain for future reference if needed.

kevinrpb avatar Jul 09 '20 10:07 kevinrpb

Fully agreed on the removal of Marshal. Incidentally, I created an app after I wrote this library and that's when I first found out about the Decodable protocol. Moving forward with the Decodable protocol is the right move.

I'll keep an eye on this as I can, but please ping me once it moves out of the draft phase. Again, thanks so much for stepping up! :)

Chris-Perkins avatar Jul 09 '20 14:07 Chris-Perkins

Hello again, @Chris-Perkins

I've pushed a few changes into this (I added explanation in the PR). I'd be great if you could take a look and tell me what you think about it.

Since you were already using structs for the results of the API calls, I thought it made sense to have them also for the request parameters (also, that's the swift way :P). This can help prevent bad encodings when making calls to the API. Unfortunately, it breaks source compatibility.

kevinrpb avatar Jul 14 '20 11:07 kevinrpb

We may also want to explicitly call out the date formatting method in the JSON Decoder; not sure if the default one is compatible with Twitch's format

Chris-Perkins avatar Jul 14 '20 23:07 Chris-Perkins

After looking at your comments it seems clear to me that it's a good idea to be explicit about the coding keys for each Codable type. I'm gonna open the API docs and review all the variable names/keys to make them match.

I also saw there are some changes to the pagination you mention. When I'm done with params/result types I'll take a look into that. Maybe we can handle it using a single PaginationData<T> object? (with generics) 🤷🏽‍♂️

As for the Encoder/Decoder date format, agreed. I'll take a look into it too.

kevinrpb avatar Jul 18 '20 12:07 kevinrpb

I was able to go through all the Params files and checked them agains the docs. Tomorrow I'll go over their Data counterparts. In the meantime, if you read this, I've been thinking about a couple things:

  • Is there any reason why there's some support for the v5 API? Should we try to move towards supporting only the New API?
  • In that sense, I will be opening an issue so we can keep track of what is covered (sorry for bloating the repo 😬 it just helps me keep track of myself)
  • I've noticed some things (like the pagination you mentioned or some filtering options) have changed in the API. When this PR is done, I'll get to fix those things as well
  • For the docstrings I just used whatever Twitch says in the docs page. I hope this is alright. Right now what is missing is class docs, I will get to those later

kevinrpb avatar Jul 18 '20 14:07 kevinrpb

Hi,

So the v5 API endpoints were added as at the time I wrote the library, there were no such replacement API calls in the New Twitch API. I needed this endpoints specifically, so I added them for my own uses.

No issues about keeping track of work items in this. Feel free to add them as you feel necessary. :)

Thanks again for all the work, this is a large task, and really grateful you could take up the torch to make the library better.

Chris-Perkins avatar Jul 18 '20 16:07 Chris-Perkins