Closed joshuamorony closed 2 weeks ago
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.
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();
});
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.
Hello @joshuamorony Is this a breaking change? If yes, can you add it in the commit description?
@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
Oh, okay.
cc @nartc
@eneajaho I figure it's probably useful to include the breaking change message either way, so I've edited the commit with the message
@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
@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
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 ;)
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.
TLDR: This removes
actionEffects
entirely (this has always been marked as experimental) and adds a deprecation warning foreffects
(which was marked as stable)Both the
effects
andactionEffects
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 keepsignalSlice
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, andactionEffects
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 Angulareffect
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 theactionSource
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). AnactionEffect
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:
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: https://github.com/ngxtension/ngxtension-platform/pull/363)), but in any case I think that
actionEffects
is not a good API and makes the typing too complex.