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

Testing a klyva-app #12

Closed
sesam opened this issue Dec 11, 2020 · 7 comments · May be fixed by #15
Closed

Testing a klyva-app #12

sesam opened this issue Dec 11, 2020 · 7 comments · May be fixed by #15

Comments

@sesam
Copy link

sesam commented Dec 11, 2020

If we assume jest, and optionally some jest-extension, what developer experience should there be for testing an app built with klyva?

Maybe something like klyva.resetAll() to move klyva state to initial? How would this be done with a redux-based app?

@merisbahti
Copy link
Owner

Ooh interesting idea.

I think in a redux-based (or any app that contains the state within a react context-provider, such as jotai and recoil), the state of the "store" is restored between tests, because a new Provider is loaded.

I'm thinking this would be really nice, but I'm wondering about implementation details.

Approach 1: A global callback, i.e. klyva.resetAll()

For us to make a resetAll-function, we need first store the "first" stored value for all "primitive" atoms, which is fine. But then we need to subscribe to a global "reset" event, which I wonder how we can do without memory leaks, the subscription needs to end when the atom ends? Possibly, we can use a Weak map or atom, I'm not sure, since they are not iterable.

Approach 2: Use jest as a platform

It's possible to use jest resetmodules or isolatemodules (https://jestjs.io/docs/en/jest-object.html#jestresetmodules) to accomplish this. How do we do this neatly? We at least need to make some official documentation that shows how to d othis.

@sesam , how do you feel about these approaches?

@merisbahti
Copy link
Owner

merisbahti commented Dec 12, 2020

Just sketching here, but a really cool way of doing this would be something like this:

  1. Klyva internally contains a let resetVersion = 0, klyva internally exports a subscribeToResets()-function which notifies listeners about when resetVersion changes, AND klyva exports a resetAll() or similar, which increments resetVersion and notifies subscribers of resetVersion.
  2. Klyva PrimitiveAtom's always store the value that they were constructed with constantly. They also keep a local state which says which resetVersion they were created at.
  3. Klyva PrimitiveAtoms when subscribed to, subscribe to changes to the resetAll, and unsubscribe when unsubscribed to. When resetAll is called, they 1. reset to their initial value, and 2. update their resetVersion
  4. When getValue is called on a primitive klyva atom, it FIRST checks if the resetVersion is the same as the previous resetVersion, if not, they skip all other work and just reset to their initial value.

This actually elegantly fixes the "dream code" scenario of just providing a klyva.resetAll() without any memory leaks.

@merisbahti
Copy link
Owner

I don't think this is a bad idea,

From a memory-management perspective this is optimal:

  1. Only base atoms store the initial value, all other derived atoms are just derived from it.
  2. The base atoms only store a reference to the value anyway.

@krawaller
Copy link
Collaborator

I'll have more confidence in my opinion once I've added tests for the components in the Klyva TodoMVC implementation, but spontaneously I'm sceptical towards a .resetAll method from a testing perspective.

What would the gains be? Sharing atoms between tests just to call .resetAll in each is functionally the same as just giving each test a new atom instance. Except in the former scenario you run the risk of accidentally sharing state.

Right now in the TodoMVC implementation we simply pass atoms down from the top of the component pyramid, meaning the atoms are received as props making testing a breeze. Since splitting atoms is SO easy with Klyva, I imagine prop-drilling will feel like less of a hassle, and that you can get away without a provider mechanism even in medium-sized apps.

In a large app I would likely just provide the central atom via a global system. If React I'd use context and a Provider, and for the tests I'd wrap Testing Library's render method in a testRender util that takes the initial atom state I want to provide, just like I would do for a Redux app (see example here).

But it might well be that I just miss the point and the advantages of .resetAll! 😄

On that topic - say that .resetAll did exist, what would a test using that look like?

@msakrejda
Copy link

+1, I think starting fresh with new state is conceptually much simpler than trying to reset an existing state. In React I really like context-based state management, because it's very easy to reason about for testing, and also generally easy to manually set up whatever state you need in a particular situation. I'm skeptical that prop drilling would be practical in more than a trivial app, but maybe @krawaller's right about klyva's advantages here.

@krawaller
Copy link
Collaborator

I'm skeptical that prop drilling would be practical in more than a trivial app, but maybe @krawaller's right about klyva's advantages here.

Hmm. Actually I've come around and think you're right, @uhoh-itsmaciek. I'll try rewriting the TodoMVC example with context instead to see how it feels.

@sesam
Copy link
Author

sesam commented Jun 29, 2021

how to do without memory leaks

A job for WeakMap perhaps? :)

I use resetAll (jest.resetAllMocks) to get back to the initial state with minimal test code. I agree it might be an edge case in many situations.

I had thought that klyva-based code would avoid prop drilling / triggering a Context that in turn alerts the whole tree below. I was thinking that performance profiling a klyva app would "light up" only components subscribed to something that changed.

I didn't think through how to inject / replace with fresh atoms in a tree of components, but I guess avoiding testing too deep structures at once is a good idea, then I guess reset is less needed. Probably best to leave the related PR unmerged until we find someone really needs it :)

@sesam sesam closed this as completed Jun 29, 2021
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 a pull request may close this issue.

4 participants