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

feat: Add Controller.getState() #2252

Merged
merged 1 commit into from
Nov 3, 2022
Merged

feat: Add Controller.getState() #2252

merged 1 commit into from
Nov 3, 2022

Conversation

ntucker
Copy link
Collaborator

@ntucker ntucker commented Nov 3, 2022

Fixes #1468 .

Motivation

Imperative handlers sometimes need context to choose their next behavior. It's critical we don't lock this into the React lifecycle as that is an antipattern (the useEffect-trap) - not just annoying from a coding perspective.

Solution

We already have imperative access with middlewares, so use the same function to provide to controller.

It's important we do not hide getState() inside any accessors, as it is critical to understand where state comes from so it always represents the expected value.

const controller = useController();

const updateHandler = useCallback(async (updatePayload) => {
  const response = controller.fetch(MyResource.update, { id }, updatePayload);
  const denormalized = controller.getResponse(MyResource.update, { id }, updatePayload, controller.getState());
  redirect(denormalized.getterUrl);
}, [id]);

Open questions

controller.getState() won't include updated state til the providers useEffect() is called. This means it could potentially not have the result. We might need another promise to listen to next state update to complete this functionality.

const denormalized = controller.getResponse(MyResource.update, { id }, updatePayload, await controller.getNextState());

Until this is resolved, one can likely just use a useIdleCallback() as the lower priority nature compared to useEffects should guarantee running after the update.

@ntucker ntucker requested a review from notwillk November 3, 2022 17:58
@ntucker ntucker changed the title Feat/controller getstate feat: Add Controller.getState() Nov 3, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 3, 2022

Codecov Report

Merging #2252 (7ccf073) into feat/getstate (3061187) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@              Coverage Diff               @@
##           feat/getstate    #2252   +/-   ##
==============================================
  Coverage          98.54%   98.55%           
==============================================
  Files                114      114           
  Lines               1860     1866    +6     
  Branches             273      273           
==============================================
+ Hits                1833     1839    +6     
  Misses                10       10           
  Partials              17       17           
Impacted Files Coverage Δ
packages/rest/src/createResource.ts 91.66% <ø> (ø)
packages/core/src/controller/Controller.ts 95.14% <100.00%> (+0.09%) ⬆️
...core/src/react-integration/provider/CacheStore.tsx 100.00% <100.00%> (ø)
...act-integration/provider/ExternalCacheProvider.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ntucker ntucker force-pushed the feat/controller-getstate branch 2 times, most recently from 7ccf073 to 937c549 Compare November 3, 2022 18:51
Base automatically changed from feat/getstate to master November 3, 2022 22:23
@ntucker ntucker merged commit 20aa9e4 into master Nov 3, 2022
@ntucker ntucker deleted the feat/controller-getstate branch November 3, 2022 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

controller.fetch or useFetcher of Resource.detail() are not instanceof Resource
3 participants