-
Notifications
You must be signed in to change notification settings - Fork 2k
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
State: Use getHttpData() for geolocation data #24631
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
688510a
State: Use getHttpData() for geolocation data
dmsnell 46d2e84
Make `fromApi` lazy and skip parser creation
dmsnell 9df67f6
Fix merge conflict
dmsnell d78abea
Update getter for thunk `fromApi`
dmsnell 4f36209
Update selector test code
dmsnell 16c766c
Rename and move files around
dmsnell f1f547d
Fix bug in getter
dmsnell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
/** @format */ | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import { http as rawHttp } from 'state/http/actions'; | ||
import { requestHttpData } from 'state/data-layer/http-data'; | ||
|
||
export const requestGeoLocation = () => | ||
requestHttpData( | ||
'geo', | ||
rawHttp( { | ||
method: 'GET', | ||
url: 'https://public-api.wordpress.com/geo/', | ||
} ), | ||
{ fromApi: () => ( { body: { country_short } } ) => [ [ 'geo', country_short ] ] } | ||
); |
File renamed without changes.
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking loudly here: a
mapStateToProps
function that doesn't usestate
and yet it works looks strange and suspicious. I didn't trust it until I understood theHTTP_DATA_TICK
trick 😄There's an opportunity for a more friendly API that uses the popular "render prop" approach:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on a similar note: if we made HttpData a Context Provider using React 16.3 APIs then we could avoid the data tick and still have the flexibility for either HoCs or render props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I'm open to update later but I don't like the "render prop" approach because it seems to add a lot of code when we already have a place to put things like this that's normal.
connect()
is Redux-oriented but it's also serving a very specific purpose for us: what are the data objects that we need, what are the side-effecting functions that we need?To me it makes sense to add into
mapStateToProps
because that's how I think many of us think about what role it's fulfilling; that or create a customconnect()
wrapper but in my explorations there I've found it gets much more complicated thatgetHttpData()
needs to be.