Skip to content
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

ResourceLoader return state selector based values #31

Closed
orther opened this issue May 20, 2018 · 5 comments
Closed

ResourceLoader return state selector based values #31

orther opened this issue May 20, 2018 · 5 comments
Assignees
Labels
enhancement New feature or request
Projects

Comments

@orther
Copy link
Member

orther commented May 20, 2018

Overview

Currently the ResourceLoader component returns a result property in the object passed to the child render prop for accessing the resource or entities the component loaded. result holds a value returned from the resource request action. This means any updates to the resource or entities redux state wont effect the result value. The problem is that in many (most?) cases after loading a resource you would actually want to render the data from the global state so your view will update if there are any state changes. For example if you load a list of resource entities to render an overview that has button to delete one. A resource delete request will update the state. A situation where you would want to use the actual resource request results is backend paginated search forms.

Current Usage Example

The following is a contrived example with a <DeleteUserButton ... /> that when clicked triggers a resource delete request for that user. The problem arrises when the delete request is successful but the user list doesn't update to not include the deleted user.

// NOTE pretend UserDeleteButton has a container that provides deleteUser
const UserDeleteButton = ({ userId, deleteUser }) =>
  <Button onClick={() => deleteUser(userId)}>Delete User</Button>

const UserList = ({ userList }) =>
  <ul>
    {userList.map(user => (
      <li key={user.id}>
        {user.name}
        <UserDeleteButton userId={user.id} />
      </li>
    ))}
  </ul>;

const UserOverview = () =>
  <ResourceListLoader resource="user" entityType="user" loadOnMount>
    {({ status, result: userList }) =>
      <Block>
        {status.pending && <Spinner />}
        {status.success && <UserList userList={userList} />}
      </Block>}
  </ResourceListLoader>;

Solutions

I have several ideas on how we should solve this but it isn't yet clear which solution is best. Below are a list of ways to solve the problem:

  1. Pass down the entity IDs as a property on the render child arg object and use EntityList component to render the list.
  2. Similar to option 1 but rather than pass the IDs just pass the entity list returned from EntityList
    1. This just renders the EntityList inside the ResourceListLoader to reduce boilerplate.
  3. Update ResourceLoader to use the resourceHelpers to create selectors for the resource being loaded and use selectors to load the value passed as result.

Final Notes

I am currently considering several use cases that aren't well supported by current helpers so there is a possibility a overarching refactor will impact this use case.

@orther orther added the enhancement New feature or request label May 20, 2018
@orther orther self-assigned this May 20, 2018
@orther orther added this to To do in Work Log May 20, 2018
@orther orther moved this from To do to In progress in Work Log May 20, 2018
@orther
Copy link
Member Author

orther commented May 20, 2018

It looks like none of this extra madness would be necessary if I just fully implemented the ResourceLoader component as detailed in #19 In the issue it's explained that if the resource was normalized by setting the entityType prop then the results passed to child render prop is the entities rather than the resource(s) and so it would be default be based on state.

My current plan is to update ResourceLoader to return raw resource response data, entities data, and then update the result prop to hold either response data or entities data based on whether the resource was normalized by setting a entityType prop value.

@orther
Copy link
Member Author

orther commented May 20, 2018

Actually it would make more sense to match the resourceHelpers selectors as described in #17 and follow this conventions:

  // returned data
  resource, // <- resource data (when normalized returns entity id(s))
  result,   // <- if the resource was normalized into entities this
            //    holds the list of entities, otherwise returns the
            //    resource data (non-normalized)

With that said, we should provide access to the raw API response data too for features like pagination. So we may want to add another prop that holds extraneous values. Will consider...

@orther
Copy link
Member Author

orther commented May 20, 2018

Current thinking is added a requestResult prop that has the following data from the resource request success action payload:

const requestResult = {
      api: payload.api,
      data: payload.data,
      entities: payload.entities,
      entityType: payload.entityType,
      resource: payload.resource,
    }

@orther
Copy link
Member Author

orther commented May 20, 2018

Final thoughts before stopping for the night....

By providing the requestResult object as outlined above I was nicely able to use EntityList component to load the entities from state. By doing so updates to state reflect in the rendered list.

        {status.success &&
          locationList &&
          entities &&
          <EntityList entityIds={entities} entityType="planAppLocation">
            {locationList => {
              console.log('locationList', locationList);
              return (
                <OrgPlanAppLocationOverviewPage
                  orgId={orgId}
                  planAppId={planAppId}
                  locationList={locationList}
                />
              );
            }}
          </EntityList>}

I created a branch with EntityList and EntityDetail created inside the ResourceLoader component but I feel like that is the wrong solution. Instead a separate component should be made that simply joins the two. Branch here: https://github.com/uptrend-tech/uptrend-redux-modules/tree/resource-loader-include-entities-experimental

@orther
Copy link
Member Author

orther commented May 20, 2018

Closing since we are using the EntityDetail and EntityList components to solve this for now. Still not sure what the best path forward is to remove even more boilerplate.

@orther orther closed this as completed May 20, 2018
Work Log automation moved this from In progress to Done May 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Work Log
  
Done
Development

No branches or pull requests

1 participant