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 setSaving function. #71

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: Add setSaving function. #71

wants to merge 4 commits into from

Conversation

stephenh
Copy link
Contributor

@stephenh stephenh commented Aug 6, 2022

This helps manage some of the "make sure to resolve on unmount" that
we'd seen in this PR:

https://github.com/homebound-team/internal-frontend/pull/2417/files?w=1

This helps manage some of the "make sure to resolve on unmount" that
we'd seen in this PR:

https://github.com/homebound-team/internal-frontend/pull/2417/files?w=1
@@ -1,26 +1,27 @@
import React, { PropsWithChildren, useCallback, useEffect, useMemo, useRef, useState } from "react";

export enum AutoSaveStatus {
/** No calls are in-flight or just-recently-saved. */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added descriptions for good measure

ERROR = "error",
}

export interface AutoSaveStatusContextType {
status: AutoSaveStatus;
/** Resets status to IDLE, particularly useful if "Error" or "Done" is stale */
resetStatus: VoidFunction;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed resetStatus with the idea that it's the responsibility of the provider itself to manage when to reset/not-reset status, based on components notifying of their own/individual status.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already did do that, to an extent.

resetStatus() was a way for components to manually clear our the save status. i.e. if something failed, the user started typing again, and we wanted to show Idle/Ready instead of Error before actually firing a new save.

/** Resets status to IDLE, particularly useful if "Error" or "Done" is stale */
resetStatus: VoidFunction;
errors: unknown[];
errors: string[];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the type to string[] b/c one place was passing Errors, another strings, and to be useful to display, the client needs to have some sort of idea of what's here.

@@ -15,69 +18,47 @@ describe(useAutoSaveStatus, () => {
});

it("renders with a provider", () => {
const { result } = renderHook(() => useAutoSaveStatus(), {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these renderHook changes are just using the wrapper alias.


act(() => result.current.triggerAutoSave());
act(() => result.current.setLoading(true));
Copy link
Contributor Author

@stephenh stephenh Aug 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving over to the setLoading API.

The context still has trigger/resolve as it's internal impl that we expose to useAutoSaveStatus, but the external API of useAutoSaveStatus is now just "loading yes/no?" and internally we turn that into trigger/resolve as/if needed.

This API change is mostly driven b/c our useMutation wrapper here:

https://github.com/homebound-team/internal-frontend/pull/2417/files?w=1

Can no longer imperatively call trigger in a try with a resolve in the catch/finally. Instead we're just told by useMutation "here's the current loading+error values", which the setLoading API facilitates:

const setLoading = useAutoSaveStatus();
const [useSaveFoo, saveFoo] = useSaveFoo();
setLoading(saveFoo.loading, saveFoo.error?.toString());

@stephenh stephenh marked this pull request as ready for review August 6, 2022 15:32
@stephenh stephenh changed the title feat: Add useAutoSaveStatus.setLoading function. feat: Add setSaving function. Aug 6, 2022
@stephenh stephenh requested review from TylerR909 and bdow August 6, 2022 16:46
@stephenh
Copy link
Contributor Author

stephenh commented Aug 6, 2022

@bdow / @TylerR909 I was futzing with this to make the ergonomics of this PR cleaner:

https://github.com/homebound-team/internal-frontend/pull/2417/files?w=1

And I think setSaving is still a good/nice change, but more fundamentally I'm realizing that, with the new useMutation-based approach of that PR, we could basically remove the form-state based integration all together (b/c any formState.autoSave is calling a mutation anyway).

Because really what that PR is doing is solving save/auto-save for all mutations, not just ones that go through form-state auto saves.

So, we could pull AutoSaveStatus/provider out of form-state all together and probably just drop it directly into Beam (we didn't do this originally b/c we thought that form-state, which is "upstream" of beam, would need to control the instrumentation; but that's not the case anymore with the useMutation-based approach.)

So, could land this PR in form-state but then automatically copy/paste the auto-save-context over into Beam, and delete it from this repo (and any calls that useFormState / useFormStates are making).

Copy link
Contributor

@TylerR909 TylerR909 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge fan of how imperative setSaving(true/false) feels. Much prefer declarative trigger/resolve. But, that notwithstanding, lgtm.

Tried to write a test to check the useEffect cleanup effect to verify status gets set back to idle but couldn't get it working. I'm convinced it works, though.

ERROR = "error",
}

export interface AutoSaveStatusContextType {
status: AutoSaveStatus;
/** Resets status to IDLE, particularly useful if "Error" or "Done" is stale */
resetStatus: VoidFunction;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already did do that, to an extent.

resetStatus() was a way for components to manually clear our the save status. i.e. if something failed, the user started typing again, and we wanted to show Idle/Ready instead of Error before actually firing a new save.

@stephenh
Copy link
Contributor Author

Not a huge fan of how imperative setSaving(true/false) feels. Much prefer declarative
trigger/resolve.

Fwiw terminology-wise I'd call setSaving the more declarative of the two b/c it lets the caller say / "declare" my current status is X. Vs. the trigger/resolve which have to be called in order, usually in an ("imperative") try/catch or useEffect.

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.

2 participants