Deprecate derived units in target

[AlexandrHoroshih]

What is the current behavior:
Usage of .on method is deprecated on derived stores and to be removed in the next release - this is very good news and, i think, makes everything a lot more consistent and predictable 👍

But now usage of derived stores as a target in operators is not warned or error by effector, so the problem of unexpected changes to derived stores still persists 😢

$readOnlyStore = $store.map(v => v)

// yay! error in console!
$readOnlyStore.on(ev, () => {})

// no error :(
sample({
 clock: $store,
 target: $readOnlyStore,
})

Also i think that stores created by restore from an event or effect should be considered as derived too, since they also have explicit dependency at the moment of creation: restore(changed, null) 🤔
UPD: Since restored event state still can be used by app and can't be derived from, for e.g., state in serialized scope during SSR, it is definetly not derived (at least, in a sense similiar to mapped stores)

Please provide the steps to reproduce and if possible a minimal demo of the problem via https://share.effector.dev, https://codesandbox.io or similar

https://share.effector.dev/yqvd359F

What is the expected behavior:
Any attempt of adding a new source of updates to dervied store is forbidden and warning/error is shown.
If such behaviour is needed - it is better to create classic (not derived) store and subscribe it to changes of original derived store.

Which versions of effector packages, and which browser and OS are affected by this issue? Did this work in previous versions of effector?:
effector@22.0.0^

[zerobias]

So we need to add such validations to:

• forward to
• sample target
• guard target
• split cases

Anything else?

[sergeysova]

Maybe add validation on the kernel level? To add support for patronum

[zerobias]

This is added to the metadata of the graph node anyway, assume usage in that way:

if (unit.graphite.meta.derived) throw Error('derived unit cannot be used as a target')

Even more, this is already implemented like that

[AlexandrHoroshih]

I updated a REPL example a bit:

• dervied events in target are also not warned yet
• merge returns an not-derived unit and its direct call is not warned

https://share.effector.dev/yqvd359F

[zerobias]

@AlexandrHoroshih thanks, I wrote more tests based on your findings

[sergeysova]

Also i think that stores created by restore from an event or effect should be considered as derived too, since they also have explicit dependency at the moment of creation: restore(changed, null) 🤔

I cannot agree. restore(event, default) invented to be a shortcut for createStore(default).on(event, (_,i) => i). So it is should create a NEW store. Example use case:

const nameChanged = createEvent<string>();
const $name = restore(nameChanged, '');

But in real life you want to reset form and load data to form fields:

$name.reset(formCleanup);

spread({
  source: formLoadFx.doneData,
  targets: {
    name: $name,
  },
})

As we see it is a common store, not derived.

[AlexandrHoroshih]

I cannot agree. restore(event, default) invented to be a shortcut for createStore(default).on(event, (_,i) => i). So it is should create a NEW store

It is how it works know, yes, but same works for mapped stores now and this behavoir is still deprecated for a reason

But yeah, restored event is probably not a derived store, because its state definitely should be transferred over the network during SSR - otherwise app rendered on the client will be different from the render at server

[sergeysova]

but same works for mapped stores now

Not the same semantics.

Mapped store it the explicit contract with the single source of truth (single source of updates).

Here we proof store has no updates source except $a:

$b = $a.map(fn)

Here the same, but source of updates is $c AND $d:

$e = combine($c, $d, (c, d) => c + d)

[zerobias]

Deprecation warning will be published in the next minor release