store.destroy() performs wrong actions.

[izmaylovdev]

Why destroy method emits initialState one more time? And even more important why destroy do not call complete?
https://github.com/ngneat/elf/blob/0261ab9f8f542a21875883c61f9e1ce56e04bc00/packages/store/src/lib/store.ts#L124:L127

[NetanelBasal]

We call reset for cases where your store is defined outside, and you want to reuse the same store afterward. For the same reason, we can't call the complete method.

Note that destroy only removes the store from the global stores' repo; it doesn't destroy the actual instance.

[izmaylovdev]

And I fond out that store.complete() does nothing https://github.com/ngneat/elf/blob/0261ab9f8f542a21875883c61f9e1ce56e04bc00/packages/store/src/lib/store.ts#L133:L138 so I can't close observables.

[NetanelBasal]

Yes, you're the only one that have a use case for this

[NetanelBasal]

You're welcome to submit a PR that adds this functionality

[izmaylovdev]

I will create PR, right now I can not find normal way to just remove a store: "destroy -> unsubscribe" submits initial value to all subscribers and "unsubscribe -> destroy" throws 'ObjectUnsubscribedError'. Maybe I can add 'delete' method for this case?

[NetanelBasal]

We can add destroy({ resetStoreValue: false }) and then call unsubscribe

[marklagendijk]

This is exactly what I ran into.
I tried to destroy the store for proper cleanup during tests, but then got issues with observables being triggered one extra time.

I would say that doing a reset in destroy, but not a complete is quite counterintuitive given the semantic meaning.
On the other hand I understand that changing the default behavior would be a breaking change that you might want to avoid, although I would ask to consider doing it at some point, because intuitive behavior is quite important for a library like this.

[NetanelBasal]

I understand the confusion. It behaves like that for stores that live outside the class. Do you have another suggestion?

[marklagendijk]

Naming is always one of the trickiest things in programming, but when we do it right it makes our APIs so much better.

I can give my opinion, but I'll leave it to you to judge how useful my opinion is, and would understand it completely if you don't agree on certain points.
Also I don't know the implementation of Elf, and unfortunately don't have time to fully dive into it at this point, so I may be missing things here.

destroy

Destroy should do the following:

• Remove the global stuff for the store
• Complete in such a way that all derived observables will complete, without emitting an extra value.

implementation

I don't know the underlying implementation of elf, but I would assume that either each store has its own BehaviourSubject or there is one global BehaviourSubject from which the different stores select.

• If every store has its own, you should be able to call complete() on it.
• If there is only one global one, you could maybe give each store its own destroyed$ Subject, and use .pipe(takeUntil(store.destroyed$)) when selecting the store state from the global store. This is a common pattern.

breaking change

If what I describe above would be possible, it would be a breaking change for the library.
I can't judge if there are common use cases out there that would be affected negatively and actually break because of this change.
If there actually are use cases where the functionality as it is now is needed, opposed to what I describe, I would prefer making that functionality available under a more descriptive name.

Personally I always prefer going ahead with breaking changes if they actually improve things.
Given the ecosystem people are quite used to SemVer and usually read the changelog when they see that the major version was bumped.

[NetanelBasal]

I thought about this, and I agree. @izmaylovdev, you can open a PR that changed the destroy method to the following:

• Remove the store from the global registration
• Complete the stream

But with that change, I want the CLI to use by default the inlineStoreInClass option when using the class variation as it makes more sense.

Please pay attention to the commit convention, and mark it as a breaking change.