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

Nested effects. #1

Closed
kethan opened this issue Sep 9, 2022 · 11 comments
Closed

Nested effects. #1

kethan opened this issue Sep 9, 2022 · 11 comments

Comments

@kethan
Copy link

kethan commented Sep 9, 2022

Wow! Awesome I was expecting a micro signal from you there it is. I have an issue if I nest effects like this:

const counter = signal(1);
const double = computed(() => counter.value * 2);
const tripple = computed(() => counter.value * 3);

effect(() => {
  console.log(double.value);
  effect(() => {
    console.log(tripple.value);
  });
});
counter.value = 20;

The triple value is consoled 3 times.

@WebReflection
Copy link
Owner

can you please try again with latest?

@kethan
Copy link
Author

kethan commented Sep 10, 2022

Nope, now it logs twice.
const dispose = effect(() => console.log(fullName.value))
Is there a dispose function like in preact signals?

@WebReflection
Copy link
Owner

I think batch works nested but effects has no special logic there … will create a test case and see how preact works and fix it, but probably not today. Thanks for the issue

@WebReflection
Copy link
Owner

@kethan I can confirm preact does exactly the same, it logs:

2
3
40
60
60

After all computed caches results so it's not called twice, so I think this is expected because inner effect should execute before outer effect and when outer effect is invoked the inner one is still an effect, hence the double 60.

However, changing the test like this:

const counter = signal(1);
const double = computed(() => {
  console.log('double');
  return counter.value * 2
});
const tripple = computed(() => {
  console.log('triple');
  return counter.value * 3;
});

effect(() => {
  console.log(double.value);
  effect(() => {
    console.log(tripple.value);
  });
});
counter.value = 20;

Shows the following:

double
2
triple
3
double
triple
40
60
60

meaning the computed value 60 is executed only once. There is a tiny discrepancy here with preact, which would instead show:

double
2
triple
3
double
40
triple
60
60

but this has nothing to do with your concern, it's just a matter of when the computed gets cached. Here it's computed right away for each registered callback per that signal, in preact is computed after the inner effect happens. Apparently usignal here is "correct" though so I think we can close this?

@WebReflection
Copy link
Owner

reopening, as tests show preact got it as wrong as I did (using preact as reference) but solid got it right, and now I understand what the dispose question was about ... will think about it, as indeed solid behavior sounds about right.

where does preact exposes dispose though? is it in the signals-core module? and are developers supposed to invoke it manually?

@WebReflection WebReflection reopened this Sep 10, 2022
@kethan
Copy link
Author

kethan commented Sep 11, 2022

@kethan
Copy link
Author

kethan commented Sep 11, 2022

And btw using your codebase I made a lib that can do both Observable and Signal (like solid, voby, knockout.js, etc).

https://github.com/kethan/ulive
Any feedback or suggestion is highly appreciated.

Thanks.

@WebReflection
Copy link
Owner

WebReflection commented Sep 11, 2022

@kethan so ... you removed my copy right from code you copy pasted from this repository and you want a review of "your work"? this is not how licenses work mate, use a dependency and/or contribute here, don't do that shit instead!

there's a difference between inspired and stolen

@WebReflection
Copy link
Owner

@kethan
Copy link
Author

kethan commented Sep 11, 2022

Oops sorry i will add that. But I did mention your repo and name. I will add that in code too .

@WebReflection
Copy link
Owner

@kethan fully fixed and tested in the fully rewritten 0.3.0 🥳 thanks for filing this issue, it helped me a lot figuring out many things 👋

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

No branches or pull requests

2 participants