-
Notifications
You must be signed in to change notification settings - Fork 1
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
GraphQL Migration #38
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stigi
commented
Oct 1, 2024
stigi
force-pushed
the
ullrich/37-graphql-migration
branch
from
October 1, 2024 14:37
3b2bae1
to
9b899bb
Compare
stigi
force-pushed
the
ullrich/37-graphql-migration
branch
from
October 2, 2024 11:05
1dd12fe
to
c04f317
Compare
stigi
force-pushed
the
ullrich/37-graphql-migration
branch
from
October 2, 2024 11:14
c04f317
to
6dbdc8d
Compare
stigi
commented
Oct 2, 2024
stigi
commented
Oct 2, 2024
smeijer
approved these changes
Oct 2, 2024
Co-authored-by: Stephan Meijer <stephan.meijer@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Resolves #37
This PR replaces the
/graphql
endpoint by using/notifications
instead.This PR Removes the Graphql Datasource and replaces it with a standard Get datasource. In order to support pagination and filtering I had to extend the http client to accept query items (via
URLQueryItem
).Adjusting the unit tests was quite bothersome, as a lot of them relied on using mocks of graphql data (hello edges and nodes) which had to be recreated. Also the primitives of pagination changed quite significantly.
Breaking changes
The API for filtering by
topic
andcategory
were changed. They previously took arrays of arguments, now they only take singular arguments, as that's what's supported by the/notifications
endpoint.Internal discussion
The API for requesting notifications from the
store
object changed from calling a function to accessing a property (which is much nicer):store.notifications()
tostore.notifications()
.Drive By fixes:
Updated
Example/Example/UIKit/MagicBellStoreViewController.swift
to prevent a race condition between the initial refresh and the scroll view triggering a fetch of the next notification page. This caused the first page of notifications to be loaded twice (even before the current PRs changes).Also updated the
Cotnributing
section in the Readme to usecarthage bootstrap
instead ofcarthage update
which would not just resolve the dependencies, but also update them.Test Plan
Unit tests are run on CI. Additional manual testing steps below.
Setup
carthage bootstrap --use-xcframeworks --no-use-binaries
Running Unit Tests
swift test
MagicBell.xcodeproj
and running tests by selectingMagicBellTests
and pressing⌘+u
Running Example App
Example
:pod install
Example.xcworkspace
(generated by cocoapods) and run the app