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

🐞 IdleTimer return functions like isPrompted, isIdle should be boolean state vars, not () => boolean functions #396

Open
1 task done
donp-usbank opened this issue Oct 17, 2024 · 2 comments
Assignees
Labels
bug A verified and reproducible bug. triage Has not been reviewed yet and should not be worked on.

Comments

@donp-usbank
Copy link

What happened?

Trying to control an "Extend session" prompt dialog using isPrompted.

In React, using a Dialog component with an isOpen prop requires use of a React state variable so that the component will re-render when the state (isPrompted) is changing. However, since idle-timer provides only a function, and not a hook-based boolean state, we can't detect when the state is changing, and therefore must use our own managed state value which is set by the onActive, onIdle, and onPrompt event handlers. This makes the isPrompted function redundant and not useful.

This could be made more useful by including and managing these states internally in the useIdletimer hook and returning them along with the other return values/objects.

Reproduction Steps

1. Implement a session confirm prompt per example https://idletimer.dev/docs/features/confirm-prompt
2. Observe that although `isPrompted` function is provided by the idle-timer library, it's not used and wouldn't be useful.
3. Observe that additional state must be implemented as boilerplate in order to have a state that represents the prompted state.
...

Relevant log output

No response

Screenshots or Additional Context

No response

Module Version

5.7.2

What browsers are you seeing the problem on? Select all that apply.

No response

What devices are you seeing the problem on?

No response

Verification

  • I have checked for existing closed issues and discussions.
@donp-usbank donp-usbank added bug A verified and reproducible bug. triage Has not been reviewed yet and should not be worked on. labels Oct 17, 2024
@SupremeTechnopriest
Copy link
Owner

This would be a breaking change and will have to wait for a major version release. Have you considered using onPresenceChange? I think that will get you what you want.

https://idletimer.dev/docs/api/props#onpresencechange

@donp-usbank
Copy link
Author

Thanks @SupremeTechnopriest

Have you considered using onPresenceChange? I think that will get you what you want.

I did look at this, and no this won't get us what we want for the same reasons noted above. Even in the example for onPresenceChange, this requires boilerplate logic implementation to produce a result variable like isPrompted - but that logic is based on rules defined by the library; meaning, the fact that the meaning of isPrompted is that presence.type is 'active' and presence.prompted is true. So, 1. the fact that users of the lib would need to implement this logic which seems to be a concern of the library, and 2. the fact that these vars are still not state variables that will cause a component to re-render - means that this isn't a useful solution for displaying a confirmation prompt.

The suggestion here is to modify the library to produce true React boolean state variables as part of the return value of the useIdleTimer hook. That could be done in either a breaking way by changing the isPrompted return property from a function to a boolean, or in a non-breaking way by adding a new prompted boolean return state variable.

Either way, a true state variable is what's useful, for the reasons above.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A verified and reproducible bug. triage Has not been reviewed yet and should not be worked on.
Projects
None yet
Development

No branches or pull requests

2 participants