-
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
Conversation
whoa, that line count |
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.
The HTTP Data API looks nice 👍 It's very similar to React Suspense and their simple-cache-provider
. I expect many of its usages to be migrated when Suspense becomes real.
*/ | ||
import makeJsonSchemaParser from 'lib/make-json-schema-parser'; | ||
import { http as rawHttp } from 'state/http/actions'; | ||
import { requestHttpData } from 'state/data-layer/http-data/common'; |
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.
What if the public API of the http-data
module could be imported from the root state/data-layer/http-data
path? Leave the submodules for the internal stuff, i.e., the enhancer, reducer, effect handler etc.
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.
fine with me. I'll try and make a PR for it - seems like a good idea
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.
done 👍
url: 'https://public-api.wordpress.com/geo/', | ||
} ), | ||
{ | ||
fromApi: makeJsonSchemaParser( |
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.
Creating a JSON schema parser is quite an expensive operation. And we create a new one from scratch on every requestGeoLocation
call, don't we? Selectors are the kind of code that makes sense to aggresively optimize. Can we refactor the requestHttpData( 'geo' )
call to be a simple Map
getter?
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.
well I thought they weren't expensive ever since we made them lazy. I agree on the point about creating them on every call
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've updated requestHttpData()
so that fromApi
is to be lazy and I think that should alleviate significant load from it. now it will only evaluate the thunk if we actually need to request the data and in that case it should be a trivial part of the overall runtime
thanks for the good catch!
}, | ||
}, | ||
// we only use the short code currently | ||
( { body: { country_short } } ) => [ [ 'geo', country_short ] ] |
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.
The schema says that body
can be null
and yet we destructure it here like it could never fail.
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.
technically as part of fromApi
it should failover automatically if we were to get a type error. I will update the schema and remove the parser
@@ -10,7 +10,7 @@ import { lowerCase, upperCase } from 'lodash'; | |||
* Internal dependencies | |||
*/ | |||
import { getCurrentUserLocale } from 'state/current-user/selectors'; | |||
import { getGeoCountryShort } from 'state/geo/selectors'; | |||
import { requestGeoLocation } from 'state/data-layer/http-data/getters'; |
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.
Could we draw a more visible line between framework and application code? Inside the state/data-layer/http-data
module, there is a mixture of both.
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 was struggling to know where to put these. In some cases there will be no getter. In other cases we'll have a single function and I don't think that warrants a separate directory like geo
had before this patch.
Any ideas on organizing these functions?
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.
What about putting getGeoCountryShort
into state/geo/getters.js
or state/geo/index.js
? The getter is a new category of a "thing" in state, distincts from a selector, reducer or action.
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 thought about this but geo
was a folder all by itself and since this is only a few lines I didn't think that would be most helpful. my expectation is that many little bits of API data will converge here and so I wanted a single place, just like state/selectors
for API data.
still open here, just sharing the why of why I didn't use state/geo
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.
Then maybe state/data-getters/index.js
could be a good home for these tiny getters? My request was to separate more clearly between framework modules and app modules that use them. Whether it's state/geo
, state/data-getters
or /lib/...
, I don't really have a strong preference. What's your view about the "framework vs app" separation, do you think it makes sense to try to achieve it here?
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.
right now these are not like the data layer handlers. they are tiny utility helper functions and will be short-lived and rarely-called and the only reason I made it is because geo
gets requested twice. I think state/data-getters
can make sense. personally I don't see much conflict with the framework/app distinction because these two things (the subsystem and the helpers) are strongly coupled together; on the same note I don't care too much about changing how it's done (I just don't see reason to made a directory structure for every five to ten line function).
export default connect( state => ( { | ||
countryCode: getGeoCountryShort( state ), | ||
} ) )( localize( LanguagePicker ) ); | ||
export default connect( () => ( { countryCode: requestGeoLocation().data } ) )( |
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 use state
and yet it works looks strange and suspicious. I didn't trust it until I understood the HTTP_DATA_TICK
trick 😄
There's an opportunity for a more friendly API that uses the popular "render prop" approach:
<WithHttpData store="geo">
{ geo => <LanguagePicker countryCode="geo" /> }
</WithHttpData>
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 custom connect()
wrapper but in my explorations there I've found it gets much more complicated that getHttpData()
needs to be.
From feedback in #24631 - Make `fromApi` lazy since it gets called on every render and might be expensive - Allow `fetch` to be lazy for the same reason - Merge `common.js` into `index.js` to consolodate imports
82ac8a6
to
ed70fae
Compare
The e2e tests are failing on this as the plans page doesn't load on this live branch, eg. https://calypso.live/start?branch=state/geo-use-get-http-data It throws a console error |
Thanks @alisterscott! This branch depends on #24656 before it can merge (see the description) and I am pretty sure that error is a result of the required changes not being in master yet. I've added the |
* Framework: Iterate on `getHttpData()` From feedback in #24631 - Make `fromApi` lazy since it gets called on every render and might be expensive - Allow `fetch` to be lazy for the same reason - Merge `common.js` into `index.js` to consolodate imports * Fix logic error in `fromApi` and update to address lazy `fromApi`
92c6e13
to
8defd8f
Compare
} ), | ||
{ | ||
fromApi: () => ( { body: { country_short } } ) => [ | ||
[ 'geo', parseInt( country_short, 10 ) ], |
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 see that fromApi
has the power to update any key it wishes, not only geo
, the requested one. Or even multiple keys. Is there a use case for this flexibility?
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.
the use case may be more strongly related to my original intention to support collections but also provides any related data we want in a mostly-flat structure.
for example, if we grab a post from the post endpoint we can update the post-like value as well.
I don't use it here yet but in every iteration where I tried I was unable to get very far without allowing arbitrary data updates. any time we grab a collection of things we need to set a different value…
fromApi: () => ( { posts } ) => [
...posts.map( post => [ `posts-${ post.id }`, post ] ),
[ 'available-posts', posts.map( ( { id } ) => id )
];
collections are possible here without the type/item
distinction in some of my more filled-out-but-more-complicated incantations by use of storing the items by id and providing a group dataset as in the example above.
this is not meant to replace the data layer and grabbing posts is probably not a good use-case due to the implicit nature and complexity. we're mainly wanting to use this wherever we have reducers that listen to a single action and shove data into state, whose data are usually closely coupled to a specific component or page, and which isn't highly related to other data types in state
it's worth noting here since the question will come up eventually - what about updates vs. replacements? I figured that we can provide a function option here which feeds in the old value but so far that's not yet necessary as we have getHttpData()
which can do the same thing. until that becomes a more obvious use-case I'm leaving it out, scoping this down.
hope that is a helpful response!
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.
Thanks, that's definitely a helpful response! Requesting a collection item would look like this, I suppose?
const requestPost = id => requestHttpData( `post-${id}`, ... );
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.
in my mind it would probably end up with using either a string for a single value or a pair of strings for a type and id
const post = requestHttpData( [ `site-posts-${ siteId }`, postId ], … );
const posts = requestHttpData( [ `site-posts-${ siteId }` ] );
const site = requestHttpData( `site-${ siteId }`, … );
By not using one string it gives us the change to do a quick "grab me the collection"
export const getHttpData = id => {
if ( 'string' === typeof id ) {
return httpData.get( id );
}
const [ type, item ] = id;
return item
? httpData.get( type ).get( item )
: httpData.get( type ).values()
}
^^^ something like that snippet but actually filled out - the snippet is for the concept not the implementation
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.
note that this is all sugar over the current flat hierarchy which could possibly prove a better long-term decision because it constrains us to think in flat terms.
const post = requestHttpData( `post-${ siteId }-${ postId }`, … );
const posts = requestHttpData( `site-posts-${ siteId }`, … )
.data
.map( postId => requestHttpData( `post-${ siteId }-${ postId }` ) );
2827e04
to
2d7fcea
Compare
@jsnajdr I moved some files around and I think it will meet your requests. |
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.
Thanks for creating the state/data-getters
directory! I think that one of the benefits is to give contributors some intuition as to which chunk their code will end up in. The framework code is likely to be in build
, vendor
or another widely shared chunk, while the individual getters can be distributed in sections that use them.
2d7fcea
to
88d459b
Compare
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 like the clear division between http, API, and application that this approach provides. And of course the line count savings. Good job!
In #22549 we added the `getHttpData()` abstraction for simple network data needs. In this patch we're moving the `geo` state subtree over to this new abstraction and removing the old code it previously required. This approach is build on the data-layer ideas and extends it to store data outside of Redux.
8fe06a1
to
f1f547d
Compare
If we start going this road, how do you envision adding offline support? |
Good question @blowery! Well that's not really addressed here but it's built on the data layer and so if we ever get around to adding things like offline queuing than this will work with that "for free" Until it can successfully fetch a resource it would remain |
In #22549 we added the
getHttpData()
abstractionfor simple network data needs.
In this patch we're moving the
geo
state subtree over to thisnew abstraction and removing the old code it previously required.
This approach is build on the data-layer ideas and extends it
to store data outside of Redux.
This is the first PR to build on
getHttpData()
and is being usedto explore the new API so that we can continue to remove code
from Calypso related to simple data needs.
Testing
The geolocation code was only being used by the language picker
and the
getCurrentUserPaymentMethods()
selector. I have testedthe language picker when the
/geo
endpoint returnsGB
and Iwould appreciate it if some others in different regions could test
that as well. The same goes for the payment methods. We want
geographically diverse tests to confirm that the currencies we are
offered are ones we want.
Size Diff
http://iscalypsofastyet.com/branch?branch=state/geo-use-get-http-data