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

Bug/361 bug singleton server side stores used for email verification status #363

Conversation

myieye
Copy link
Contributor

@myieye myieye commented Nov 2, 2023

Resolves #361

Introduces a simple layer on top of the context-api and uses that to:

  1. Stop using singleton stores for email stuff
  2. Refactor/streamline the error store in the context

Copy link

github-actions bot commented Nov 2, 2023

UI unit Tests

1 tests  ±0   1 ✔️ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ±0 

Results for commit 1472ac0. ± Comparison against base commit d11d232.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

I think the context functions introduce a race condition. Combined with the fact that making the expression reactive does nothing (svelte can't detect the change).

If user page is executed first, then it'll get a null value instead of the store, then the Layout file gets executed putting the store in the context, but at this point the page has no reason to execute useEmailResult again. I'd say make a defineStoreContext that sets a value in the context when use is called and the context returns null. Basically make it init automatically from the first place it's called.

@myieye
Copy link
Contributor Author

myieye commented Nov 6, 2023

@hahn-kev I don't think I've introduced a race condition. Also, having the context auto-init on first use wouldn't work, because context is scoped to the children of the component that initializes it, so it needs to be the layout in this case.

I can't find it documented, but I think the order in which layouts and pages/components are initialized is consistent, starting at the root layout and going down. So, if a parent component initializes/sets context, it is ready and available when a child component initializes. The tutorial makes that assumption: https://learn.svelte.dev/tutorial/context-api.

I think, that if you want context to be reactive, you should make the context value a store and let the store handle the reactivity.

@myieye myieye requested a review from hahn-kev November 7, 2023 15:43
@hahn-kev
Copy link
Collaborator

hahn-kev commented Nov 8, 2023

ok, I see now, I didn't realize that Layout used EmailVerificationStatus. This does mean that the EmailVerificationStatus component is only usable from Layout.svelte or a child of Layout.svelte, otherwise it may or may not work depending on the rendering order. But I guess that's ok since this is used in the root of the app, so all pages should apply.

@myieye myieye merged commit 4076bcb into develop Nov 8, 2023
6 checks passed
@myieye myieye deleted the bug/361-bug-singleton-server-side-stores-used-for-email-verification-status branch November 8, 2023 09:23
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.

Bug: singleton server-side stores used for email-verification status
2 participants