Request for Comment: Attempt at removing react-palm for 1 task

[chrisirhc]

Approach

• Rewrite tasks into a component with effects following the Data Fetching example on ReactJS
• Uses standard react-dom/test-utils

Here's what things could be like if we don't use react-palm for side effects/tasks.

Evaluation

Pros:

• Less non-standard practices

Cons:

• Promises stubbing in sinon still seems pretty primitive, need to pre-declare all the resolves before the method is called in order to have synchronous tests. It doesn't have the niceties of task draining/resolving in order
• Since there isn't a "tasks" abstraction, tests need to know about the implementation details of the component in order to test its payloads. In this case, the TASKS exposed by the request-map-styles component.
• You lose the developer ergonomics of redux. (i.e. there isn't a redux action you can output)

Overall: The tests seem a little less readable after removing tasks. However, I guess one could introduce some sinon.spy helpers to help with readability.

What this PR is missing / left todo:

• [ ] Remove the unused tasks (request map styles)
• [ ] Remove commented out doesNotThrow call
• [ ] Update to tape@5 which includes https://github.com/substack/tape/issues/472 to support async tests
• [ ] Does not handle tasks called from a redux action (need to change this to use a useEffect and listen to changes to mapStyles object but without infinite loop): https://github.com/keplergl/kepler.gl/blob/070b04b2c02a87cab1d3e48d789c146682befa1a/src/reducers/map-style-updaters.js#L359-L361

[btford]

nice!

I think though that we need to consider this alongside whether or not Kepler will migrate off of Redux. If that is a goal for Kepler, it might be good to keep the task system in the interim, but write hook-based APIs that allow you to resolve tasks local to a component rather than as part of redux dispatch. This would possibly give you the benefit of being able to keep the tests a bit closer to what they are right now. Then, once state is moved from Redux to local component state and/or React Context, you can start to remove tasks if that's desirable.

[chrisirhc]

I ran into the receiveMapConfig action and it's leading me to think along the same lines. (A component based task resolver)

My concern around this PR's current approach is that it moves the tasks code away from code that it should be local to (locality/proximity to where the request local action and map-style-updaters).

Creating a component-based task resolver might alleviate that and also remove react-palm. It also preserves locality of the code and keeps side effects (their task effect definitions) into one area, that new component. Though not sure if the component task resolver approach is straightforward to implement. Might need to spike it to see how it goes.

Separately, IMO, removing redux seems like a larger effort/overhaul with fewer obvious benefits. But shall wait to hear what others think.

[heshan0131]

I think moving off redux at the moment is a much larger undertaking, I would rather not involve it in this PR. We can still keep react-palm task system but separate it out from redux. This means we don't really need to remove react-palm

A component-based task resolver is worth a try. One issue I see with component-based task resolver is that you always have to carefully handle responses resolved after the component is unmounted.

Noting that we are also using react-palm to load files from the file uploader. we will have to move that logic into the file upload component.