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

Unify event architecture between api and keystore #489

Open
wants to merge 3 commits into
base: session-issues
Choose a base branch
from

Conversation

emlun
Copy link
Member

@emlun emlun commented Dec 6, 2024

With #483, the api and LocalStorageKeystore modules now expose two very different ways of attaching event listeners to these services. This PR aims to unify both APIs under a shared architecture based on the EventTarget type available in the JS engine.

As part of that, this also introduces util.cleanupListeners to help streamline setting up useEffect cleanup handlers to remove stale event listeners. This helper function sets up an AbortController and provides its signal so that event listeners can be automatically removed using the signal option of addEventListener.

@emlun emlun requested a review from gkatrakazas December 6, 2024 16:34
@emlun emlun mentioned this pull request Dec 6, 2024
@gkatrakazas
Copy link
Member

gkatrakazas commented Jan 16, 2025

Hello, @emlun , i did one simple test

  • Open tab_1 and sign up as test1.
  • Open tab_2 and login as test1.
  • Click Logout from tab2 . This causes correct tab_2 clean Session State, events listen etc, on tab_1 closes keystore but never logout and clean session (as expected).

Take a look on my PR #488, and don't consider the second commit about freshlogin ef4dc59 i need to find a better approach on separated pr,

take a look on the comments i left #488 (comment)

I don't know if tried to fix with the proper way but i have describe the issues i was tried to fix.

@emlun
Copy link
Member Author

emlun commented Jan 17, 2025

Ok, sorry for leaving this hanging over the conference and the holidays. How would you like to proceed? I'm much too out of the loop on these, so I'm happy to let you decide what to do. 🙂

  • Click Logout from tab2 [...] but never logout and clean session (as expected).

Just for clarity, by "as expected" here did you mean "the expected behaviour is that this happens, but it did not"? (as opposed to "it is the expected behaviour that this indeed does not happen")

@gkatrakazas
Copy link
Member

Apologies for the misunderstanding. By the expected behavior, I mean that the sessionState does not cleared.

I want to do some more tests on your pr and on #488 and remove for sure the ef4dc59

if you have any comment or suggestion about the logout/cleaning proccess on #488 will be helpful

@emlun
Copy link
Member Author

emlun commented Jan 20, 2025

I'm pretty sure that right now you know much better than I how these things fit together 😄 so no, I don't think I have much to add at this point. But please let me know if there's something you want me to investigate.

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