-
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
Framework: Get HTTP data #22549
Framework: Get HTTP data #22549
Conversation
31e5423
to
4cdec02
Compare
@@ -258,6 +258,8 @@ export const HELP_TICKET_CONFIGURATION_REQUEST_FAILURE = | |||
'HELP_TICKET_CONFIGURATION_REQUEST_FAILURE'; | |||
export const HELP_TICKET_CONFIGURATION_REQUEST_SUCCESS = | |||
'HELP_TICKET_CONFIGURATION_REQUEST_SUCCESS'; | |||
export const HTTP_DATA_REQUEST = 'HTTP_DATA_FETCH'; | |||
export const HTTP_DATA_TICK = 'HTTP_DATA_TICK'; |
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's the purpose this TICK action is serving? Is it to force updates for redux connected components?
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.
you got it. without dispatching then none of the listeners will know things updated
|
||
window.httpData = httpData; | ||
window.getHttpData = getHttpData; | ||
window.requestHttpData = requestHttpData; |
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.
Why add these to the window global? What do you have in mind for this?
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.
oh just for development and testing - they will need to be removed. I've left them there because they fail the CI builds, a good reminder not to merge them into master
right now in the dev console though you can call them directly to interact with the system
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.
Okay, thanks
I like this approach, but I wish it didn't involve triggering requests directly in lifecycle methods still. I feel that's something we should try to get away from if we can. |
what do you mea @coderkevin? this isn't addressing the question of where we request data. it could be put into a query component just the same but also I've thought that ideally it would go into a wrapper around |
|
||
export const getHttpData = id => httpData.get( id ) || empty; | ||
|
||
export const requestHttpData = ( id, action, { fromApi, freshness } ) => { |
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 state/data-layer/wpcom-http/utils
you gave the options bag a default which makes the control logic a bit simpler. e.g. fromApi
would be the identity function. Would you want to do that again 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.
when I woke up on the 16th I thought to myself, "self, let's try something more performant here." maybe it's best to leave in the identity 🤷♂️
|
||
const parseResponse = ( data, fromApi ) => { | ||
if ( 'function' !== typeof fromApi ) { | ||
return 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.
bug - should return a list
}; | ||
|
||
const onSuccess = ( action, apiData ) => { | ||
const [ error, data ] = parseResponse( apiData, action.fromApi ); |
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.
whats the reason for the parseResponse
function as opposed to lifting the try/catch to this function? From what I can tell, most of parseResponse is converting a try/catch into an if/else.
we could also write:
const onSuccess = (action, apiData) => {
let data;
try {
data = action.fromApi(apiData);
} catch( error ) {
return onFailure(action, error)
}
// rest of func verbatim
}
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.
only that ugly let
😉
this is pretty interesting stuff. Its cool that it skips all of the redux/data-layer/selector/reducer steps and deposits data directly to a component. I also like that items have an explicit pending state. Can you elaborate more on how you see this being used? When should someone choose |
return onFailure( action, error ); | ||
} | ||
|
||
if ( 'multi-resource' === action.fromApiType ) { |
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.
do you already have a use-case that would benefit from the multi-resource
type?
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.
getting any collection of things like posts or comments or activity log items.
I have decided that we should update the original request identifier too
component wants to know "have I fetched posts for this site yet?" and
the request id shows pending then done. however, it brings in twenty
posts. those posts may or may not be in memory when the post component
displays it. so it says, "have I fetched this post yet?" and now because
we requested it then the post is there.
also this means that the post shows up in memory whether we fetched it
from "get all posts for site" or "fetch this specific post" - it's part-and-parcel
of how the data types and ids work.
c045836
to
0d9243c
Compare
Because sometimes we don't want to store a bunch of data and yet we want to stick close to the data layer. The generalized system of handlers in the data layer presents specific challenges when React components want transient data and creating reducers, data layer handlers, and selectors requires efforts disproportionate to the task. The data layer provides centralized and optimized abstractions over network activity but also depends on Redux actions to update app state. Sometimes we also store a significant amount of data which doesn't really need to belong in our Redux state tree as it adds bloat and needless garbage collection overhead. In this patch I'm introducing the `httpData` system as a precursor to a more specific `apiData` system which hopes to improve the data layer's ability to provide structure without overhead and form a basis for a long-necessary generalized request-tracking system. This system may end up as a prototype for storing application data outside of the Redux tree and in a _more proper_ database for its needs. For instance, in many cases what we need in state is not th data itself but rather whether or not the data has changed since the last dispatch. A post object may contain many kilobytes of data but truly "belong" in IndexedDB. The `httpData` system stores nothing in `state` other than a ticker used to indicate when there are data updates. If any component is `connect()`ed into the Redux store then because `httpData` dispatches a tick then `connect()` will run and we have the change to refresh the data from the external system. In this case it's a `Map()` and the update should be extremely fast. Data is referenced by a provided `id` and the namespace is flat. The `id` should be a string probably and we should standardize formats. ```js // type-id1-id2-id3 `post-${ siteId }-${ postId }` ``` The ids need to be unique to the resource. Surprisingly we can reference a resource before it has been requested due to the consolidation of references with the resource id itself. This also opens some other questions due to the fact that different requests can provide data to the same id. It does not matter what kind of request `httpData()` tracks. It merely passes on the `fetch` property of the action into the dispatcher. In most cases this will be a WordPress.com API request with the `http()` helper but it could also be a raw HTTP request with the raw helper. An optional `fromApi` function can be provided to parse and transform the HTTP response. Currently the data pool will not be cleared and will continu to accumulate as the application runs. As we discover need we should be able to easily enhance the middleware with a pruning routine to clear out old data. If a component is rendering data which would be cleared then we would fire another tick and the component could decide to re-issue another request to update the data. Freshness can be maintained through examining the metadata and we could easily add a flag if we felt it was warranted. ```js componentDidUpdate() { const { postId, postIsStale, requestPost, siteId } = this.props; if ( postIsStale ) { requestPost( siteId, postId ); } } const mapStateToProps = ( state, { postId } ) => { const siteId = getSelectedSiteId( state ); const post = getHttpData( `post-${ siteId }-${ postId }` ); const isPending = post.status === 'pending'; const postAge = Date.now9) - post.lastUpdated; return { siteId: getSelectedSiteId( state ), post: post.data, postIsStale: ! isPending && postAge < 5 * MINUTE_IN_MS, } }; const mapDispatchToProps = ( { requestPost: ( siteId, postId ) => requestHttpData( `post-${ siteId }-${ postId }`, requestSitePost( siteId, postId ) ), } ) ``` This should not replace commonly-used and well-defined data types and their data-layer handlers. It's more for one-off requests and transient data. The example is there only for illustrative purposes.
0d9243c
to
7b75ac4
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 think the concept is promising here. Let's follow up in additional PRs:
- unit tests
- a readme to explain the use cases and why. Please also update https://github.com/Automattic/wp-calypso/blob/master/docs/our-approach-to-data.md
- a first example PR
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.
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.
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.
* State: Use getHttpData() for geolocation data 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.
Prior work in #19060
TL;DR
p4TIVU-86D-p2
This is a read-only abstraction built upon the data layer to make it easy to add "shove it" kinds of data into Calypso from an API call without needing to write reducers, query components, data layer handlers, and other common snippets traditionally required for new data sources.
Do you have a reducer that does nothing more than store data opaquely on a single action type? Do you have a query component you don't think you need? This is for you
http://iscalypsofastyet.com/branch?branch=framework/get-http-data
Because sometimes we don't want to store a bunch of data and yet we want
to stick close to the data layer.
The generalized system of handlers in the data layer presents specific
challenges when React components want transient data and creating
reducers, data layer handlers, and selectors requires efforts
disproportionate to the task.
The data layer provides centralized and optimized abstractions over
network activity but also depends on Redux actions to update app state.
Sometimes we also store a significant amount of data which doesn't
really need to belong in our Redux state tree as it adds bloat and
needless garbage collection overhead.
In this patch I'm introducing the
httpData
system as a precursor to amore specific
apiData
system which hopes to improve the data layer'sability to provide structure without overhead and form a basis for a
long-necessary generalized request-tracking system.
This system may end up as a prototype for storing application data
outside of the Redux tree and in a more proper database for its needs.
For instance, in many cases what we need in state is not th data itself
but rather whether or not the data has changed since the last dispatch.
A post object may contain many kilobytes of data but truly "belong" in
IndexedDB.
The
httpData
system stores nothing instate
other than a ticker usedto indicate when there are data updates. If any component is
connect()
ed into the Redux store then becausehttpData
dispatches atick then
connect()
will run and we have the change to refresh thedata from the external system. In this case it's a
Map()
and theupdate should be extremely fast.
Data is referenced by a provided
id
and the namespace is flat. Theid
should be a string probably and we should standardize formats.The ids need to be unique to the resource.
Surprisingly we can reference a resource before it has been requested
due to the consolidation of references with the resource id itself.
This also opens some other questions due to the fact that different
requests can provide data to the same id.
It does not matter what kind of request
httpData()
tracks. It merelypasses on the
fetch
property of the action into the dispatcher.In most cases this will be a WordPress.com API request with the
http()
helper but it could also be a raw HTTP request with the raw helper.
An optional
fromApi
function can be provided to parse and transformthe HTTP response.
Currently the data pool will not be cleared and will continue to
accumulate as the application runs. As we discover need we should be
able to easily enhance the middleware with a pruning routine to clear
out old data. If a component is rendering data which would be cleared
then we would fire another tick and the component could decide to
re-issue another request to update the data.
Freshness can be maintained through examining the metadata and we could
easily add a flag if we felt it was warranted.
This should not replace commonly-used and well-defined data types and
their data-layer handlers. It's more for one-off requests and transient
data. The example is there only for illustrative purposes.
For testing and development purposes only I have pushed in an inappropriate
usage of
httpData()
in the Activity LogTesting
Open a site in the My Sites > Stats > Activity
This must be done in a local development environment.
Notice that at the top of the list of Activity Log events there
is no ugly and unprofessional message.
Request some random yummy HTTP data from the console dispatcher:
We can return multiple independent resources if we specify
As soon as the network request returns you should see the ugly message.
Update I have implemented data freshness in this PR