fix(signal-slice): simplify api to deal with typing issues

[joshuamorony]

TLDR: This removes actionEffects entirely (this has always been marked as experimental) and adds a deprecation warning for effects (which was marked as stable)

Both the effects and actionEffects configurations introduce a great deal of typing complexity, some of these issues are (likely) fundamentally unsolvable without making the utility more complex. I think it makes sense to keep signalSlice as a simple utility, rather than having it scope creep into a full state management library. effects was really only ever for convenience/style anyway, and actionEffects is of questionable value.

The convenience of the effects configuration is in my opinion not worth the complexity/issues it introduces.

The actionEffects configuration was added as a way to trigger a side effect as the result of some source/actionSource emitting without needing to introduce new values into the state signal (which would then allow creating side effects using the standard Angular effect to react to these values being set in the signal)

However, the current implementation is not a total solution anyway, because if your actionEffect requires some value to be passed in it will mean the actionSource needs to emit that value, which means that it would still need to be added to the state signal anyway (because emissions from action sources are treated as partials that update the state). An actionEffect really only achieves its purpose for situations where you want to react to some action without needing to pass any values.

Overall, I think the actionEffect API is not a full solution in the first place and it also introduces a lot of complexity which is why I favour removing it.

This does make side effects for "events" a little more difficult, however it is still possible to do by setting version values into the state signal via action sources and then using standard signal effects, e.g:

const initialState = {
  status: 'initial',
  uploadAudioComplete: 0,
}

effect(() => {
  console.log(state.uploadAudioComplete());
})

The actionSource you want to create a side effect for would just need to increment the relevant property in the state signal.

It may be a good idea to revisit alternatives in the future (e.g. maybe we can just add something that automatically does this "versioning" behind the scenes and exposes it to react to (edit: I've put up a PR for this: #363)), but in any case I think that actionEffects is not a good API and makes the typing too complex.

[michael-small]

TLDR: This removes actionEffects entirely (this has always been marked as experimental)

I didn't use this due to its experimental-ness and questionability in general. I would say this is a fair change. #363 looks like a fair alternative.

adds a deprecation warning for effects (which was marked as stable)

I have been enjoying the dedicated effects block for it declarativeness and co-location with the relevant slice. That said, the concern about signalSlice staying more of a utility than a fullblown state management solution as well as the typing complications are fair points.

One idea that is probably overkill but it seems adjacent to #363: some <topLevelSliceProperty>Updated or <sliceSelector>Updated would be a natural tie-in for making effects on the slice in my opinion; if possible/not equally as cumbersome to manage from a utility complexity standpoint. Declaring effects on the slice without the dedicated effects piece isn't that necessary, but it is nice to have some sort of formal distinction between an effect on the slice vs a general effect that may be on a different piece of signal state in a given file. For example, my usage of signalSlice has been in my team's normal convention of a service with a signal for forms. There are other signals in those services that do their own thing that aren't directly tied to the slice, and those could have their own effect needed. Having some sort of formal bridge like an <sliceProperty>Updated if possible would be a nice distinction between the different types of effect. We use the slice's effects like your video about forms and the slice, so we do have some existing usages that are coupled.

[joshuamorony]

Thanks for the feedback!

I do agree that it's nice to have the effects option within signalSlice and would prefer to keep it, but given the options which are really:

1. Leave it as is, but if someone tries to define effects before other configurations it will break the typings in confusing ways. This just feels a bit broken and incomplete to me
2. Complete API re-design, likely involving NgRx Signal Store style functions instead of a configuration object (e.g. withSelectors(), withEffects())

I think given the effects configuration is essentially a nice to have and really just a duplication of what effect already does it makes the most sense to get rid of it.

I'm not sure I'm completely following your idea with also having an Updated signal for selectors/state as well, would you mind showing a hypothetical/pseudocode usage?

I suppose if you have both signalSlice state and non-signalSlice state in a service there would still be at least some sort of distinction in the effects since you would be accessing the signals on the slice, e.g:

effect(() => {
  mySignalSlice.someSelector();
});

vs

effect(() => {
  mySignal();
});

[michael-small]

I wrote that response barely out of bed so to be honest I don't know exactly what I was getting at either. I think I was just looking at the other PR's alternative changed syntax and was like "that's cool, can that fill some sort of similar goal for regular slice state in regular non slice effects". But as you point out, it isn't necessary since you can just tap into those directly by the nature of signals lol.

Overall, the way you define those options now I agree it's worth dropping. And I discussed this PR today with a coworker and they also agreed that dropping the effects of the slice in favor of just using built in Angular effect is a fair call.

[eneajaho]

Hello @joshuamorony
Is this a breaking change? If yes, can you add it in the commit description?

[joshuamorony]

@eneajaho do we consider the removal of APIs marked as experimental as breaking?

actionEffects being removed would be breaking in that case, but effects (which was marked as being stable) just has a deprecation warning added and has not actually been removed yet

[eneajaho]

Oh, okay.

cc @nartc

[joshuamorony]

@eneajaho I figure it's probably useful to include the breaking change message either way, so I've edited the commit with the message

[joshuamorony]

@nartc I'll double check using the anonymous function for passing the config works as expected, and if so I'll update this PR to remove the deprecation of effects and rename this PR to just remove actionEffects.

Then I'll put up a separate PR for making the breaking API change from signalSlice({}) to signalSlice(() => ({})) and I assume it should be pretty easy to include a migration for this as well

Will mark this as draft for now and ping when this one is ready

[joshuamorony]

@nartc nevermind, I was mistaken about the API change, the issue persists anyway (and I'm pretty sure there is no simple way to get around the type inference issue). We can proceed with the original plan of removing actionEffects + deprecating effects.

EDIT: in case we want to dig this up again in the future, this tweet contains context for the underlying issue: https://x.com/Nartc1410/status/1800574473097371946 and Matt Pocock also wrote an article about it: https://www.totaltypescript.com/property-order-matters

[Pilpin]

Hello everyone and thank you for a great repo full of amazing tools !

Quick question, is there a roadmap for the release of this PR and #363 ?

Thanks again ;)

[joshuamorony]

From my end this is all good and should ready to be merged (there haven't been any changes since Chau's previous review). After this is merged I will update/check/review #363 but that should also be ready to go.