fix(computedFrom): add initialValue + throw Error in case of not sync emit

[dmorosinotto]

Hi @eneajaho I've finally find the time to make some changes to computedFrom as prompted in #37

1. My first commit add some more tests to your current implementation to prove the issue that I have with "spurious" sync emit of null or initial input in case where observable don't have a valid sync emitted value.

You document those cases as tricky part in the docs, but I believe that we can handle it differently throwing an error as the "native" Angular toSignal do, so the developer knows where his code will generate the problem and decide how to fix it!

2. I changed the computedFrom implementation and tests to handle the initialValue: Output passed in options object as last parameter (the options object can be used even to pass the injector as before) - I rewrote the test that use the new behavior.

3. In the last commit I updated the docs to explain these new behaviors: initialValue + throwing Error.

I hope you can review this PR and give me some feedback, because in the old Issue we didn't have the opportunity to discuss much about how to proceed, and so I decided to complete the code and tests to have a discussion base, and maybe fix or improve it even more to have a "better" computedFrom 🤓

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a2a8c37. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

⌛ The following target is in progress

• nx affected --target=build --parallel=3 --exclude=docs

✅ Successfully ran 3 targets

• nx affected --target=test --parallel=3 --ci --code-coverage
• nx affected --target=lint --parallel=3
• nx-cloud record -- pnpm exec nx format:check

---

Sent with 💌 from NxCloud.

[eneajaho]

Hello @dmorosinotto

Thanks for this!!

[dmorosinotto]

Hi @eneajaho sorry to directly push my code into PR without previous discussion in the issue #37

But if you agree we can discuss/refine the idea about computedFrom here directly on the code side 😉

I added “many comments” to explain my ideas and motivation, obviusly we’ll remove them befote merge, but let’s start the discussion and give me any feedback about my changes, even what you think about the breaking behaviour of throwing error 👀

[dmorosinotto]

Hi @eneajaho , @nartc I know you are both very busy following all the latest Angular news & community, but is there a chance someone would take a look at this PR and gives me any feedback or maybe start a discussion on what you don't like, to make it progress and eventually the changes needed before merging it?

Thanks in advance and sorry to bother you, I really just want to be collaborative... not criticism!

[eneajaho]

@nartc What do you think about this one?

I think we will need sth like this.

[dmorosinotto]

@eneajaho when you ask me to open a new issue to discuss try catch handle error are you referring to those commented lines where I suggest a way to handle the error due to requireSync to customize the message for computedForm?

[eneajaho]

Hi @dmorosinotto

It looks good to me. It doesn't look breaking to me. There are some comments I'd like you to review.

And we need to discuss the error-handling part with @nartc .

Thanks for your patience.

[dmorosinotto]

Hi @eneajaho thanks for your feedback reviewing my code, today I'm quite busy at work, but I'll take all your suggestions to update my code this evening (after 21:00 CEST)
Just to give you some quick response (than I'll detail it in context of your comments)

• I'll remove/lower all UPPERCASE comments I used them intensively just to explain my reasons for the changes/behaviors in which I changed the code, since we had not had a chance to discuss the changes I made beforehand
• about the reverted initialValue sample where in your original implementation we get the [1,2] emission, it's not possible because in case of "async later emit" the new code will throwError becouse I added pipe(delay(1000)) to made [signal(1), singal(2)] loosing sync value due to delay operator.
• about the MD & MD FIX in the tests title it was just my signature to easly find/quick identify where I changed the behavior / breaking change respect to your older behavior, I'll absolutely remove MD from the final code!
• I don't know where I added the exp = required("...") probably some wrong auto-import, obviously I'll remove it
• My code introduce a "breaking change" (that I try to demonstrate with tests before, and after) was eliminating null + initial spurious emit of array/object as initial value in case of late emit preferring to throw error if the dev use computedFrom without passing initialValue param, because old behavior in either case will hide possible run-time problem that TS will not catch, because the infer Signal type never include the null or initial Arr/Obj ... sorry my bad english, hope you understand what I mean, or just check the 1st commit tests where I demonstrate it with expect toThrowError with comments like //NOTICE THAT THIS WILL EXPLODE AT RUNTIME - TS DON'T CATCH IT!!! sorry for uppercase but it's just to find it 😉

[dmorosinotto]

OK @eneajaho I've tried to fix all the typos and uppercase tests comments, hope this will made it more readable and conform to the repo standards.

Waiting for your feedback and @nartc opinion about the new throwing error behavior 👀

[dmorosinotto]

@eneajaho , @nartc I fixed the CI errors - if you want to review the code and give me some feedback about what you think of the current "throwing error" behavior?

BTW: I had to do some "try&error" to make a commit passing CI, but running local test always pass even with previous failing CI commits, I suspect that the CI is using some stricter linting rules! Can we align those so anyone can verify all code & test works as expected from CI before commit? Thanks

PS: hope to meet you IRL at NGPoland next week, to discuss this or other Angular stuff 😉

[eneajaho]

Hi @dmorosinotto
Thanks, it looks good now.

Regarding try catch error handling, would you mind opening an issue, so we can tackle it later and get this PR merged?

Also, please can you fix those other change requests? And after that it should be good to be merged.

[dmorosinotto]

Hi @eneajaho I've added a commit to fix remaining typo in docs.

And just for clarification about the issue that you want me to open - are you referring to commented lines in computed-from.ts L113-128 where I suggest a way to handle&customize error message due to requireSync used to warn about async late emit?

[nartc]

I hate you guys for having fun at ngpoland conf without me so I rage merged this PR

[dmorosinotto]

Thanks @nartc for merging it 🙏
We really miss you at ngPoland hope to meet even you in a near future!

[eneajaho]

Oh, I'm so sorry for you @nartc 🫶