Repository: effector/effector
Pull request #897 (2023-05-13 08:59:21)

Fix sidless edge case
### Conventions
- [x] Please check your messages [against the guidelines](https://cbea.ms/git-commit/) if not, perform [interactive rebase](https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-history)
- [ ] [Link an issue](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) your pull request closes or relates


codeflowapp[bot]: <a href="https://stackblitz.com/~/github.com/effector/effector/pull/897"><img src="https://developer.stackblitz.com/img/review_pr_small.svg" alt="Review PR in StackBlitz Codeflow" align="left" width="103" height="20"></a> _Run & review this pull request in [StackBlitz Codeflow](https://stackblitz.com/~/github.com/effector/effector/pull/897)._


Patch:
diff --git a/src/effector/fork/fork.ts b/src/effector/fork/fork.ts
index ffa11105..31bbc419 100644
--- a/src/effector/fork/fork.ts
+++ b/src/effector/fork/fork.ts
@@ -33,7 +33,7 @@ export function fork(
       forEach(activeEffects, scopeRef => (scopeRef.ref = scope))
     }
     if (config.values) {
-      const {sidMap, unitMap} = normalizeValues(config.values, unit =>
+      const {sidMap, unitMap, hasSidDoubles} = normalizeValues(config.values, unit =>
         assert(is.store(unit), 'Values map can contain only stores as keys'),
       )
       Object.assign(scope.values.sidMap, sidMap)
@@ -42,6 +42,7 @@ export function fork(
       })
       scope.fromSerialize =
         !Array.isArray(config.values) && !(config.values instanceof Map)
+      scope.hasSidDoubles = hasSidDoubles
     }
     if (config.handlers) {
       scope.handlers = normalizeValues(config.handlers, unit =>
diff --git a/src/effector/fork/serialize.ts b/src/effector/fork/serialize.ts
index 45d7c236..1569ed5d 100644
--- a/src/effector/fork/serialize.ts
+++ b/src/effector/fork/serialize.ts
@@ -17,6 +17,7 @@ export function serialize(
       'There is a store without sid in this scope, its value is omitted',
     )
   }
+  assert(!scope.hasSidDoubles, 'duplicate sid found in this scope')
   const ignoredStores = config.ignore ? config.ignore.map(({sid}) => sid) : []
   const result = {} as Record<string, any>
   forIn(scope.values.sidMap, (value, sid) => {
diff --git a/src/effector/fork/util.ts b/src/effector/fork/util.ts
index b08e743f..7f9f126d 100644
--- a/src/effector/fork/util.ts
+++ b/src/effector/fork/util.ts
@@ -44,6 +44,7 @@ export function normalizeValues(
   const mapOrRecordValues: Map<StoreOrEffect, any> | Record<string, any> =
     Array.isArray(values) ? new Map(values as [StoreOrEffect, any][]) : values
   const unitMap = new Map<Unit<any>, any>()
+  let hasSidDoubles = false
   if (mapOrRecordValues instanceof Map) {
     const sidMap = {} as Record<string, any>
     forEach(mapOrRecordValues, (value, key) => {
@@ -53,13 +54,12 @@ export function normalizeValues(
       )
       if (assertEach) assertEach(key, value)
       if (key.sid) {
-        assert(!(key.sid in sidMap), 'duplicate sid found')
+        if (key.sid in sidMap) hasSidDoubles = true
         sidMap[key.sid!] = value
-      } else {
-        unitMap.set(key, value)
       }
+      unitMap.set(key, value)
     })
-    return {sidMap, unitMap}
+    return {sidMap, unitMap, hasSidDoubles}
   }
   return {sidMap: mapOrRecordValues, unitMap}
 }
diff --git a/src/effector/kernel.ts b/src/effector/kernel.ts
index 188b07e8..59498d37 100644
--- a/src/effector/kernel.ts
+++ b/src/effector/kernel.ts
@@ -496,10 +496,10 @@ export const initRefInScope = (
     meta: sourceRef.meta,
   }
 
-  if (sid && sid in scope.values.sidMap && !(sid in scope.sidIdMap)) {
-    ref.current = parser(scope.values.sidMap[sid])
-  } else if (ref.id in scope.values.idMap) {
+  if (ref.id in scope.values.idMap) {
     ref.current = scope.values.idMap[ref.id]
+  } else if (sid && sid in scope.values.sidMap && !(sid in scope.sidIdMap)) {
+    ref.current = parser(scope.values.sidMap[sid])
   } else {
     if (sourceRef.before && !softRead) {
       let isFresh = false
diff --git a/src/effector/unit.h.ts b/src/effector/unit.h.ts
index 8b7c67bb..59e9854f 100644
--- a/src/effector/unit.h.ts
+++ b/src/effector/unit.h.ts
@@ -165,6 +165,7 @@ export interface Domain extends Unit {
 
 export interface Scope extends Unit {
   fromSerialize?: boolean
+  hasSidDoubles?: boolean
   kind: Kind
   graphite: Node
   reg: Record<string, StateRef>


Test patch:
diff --git a/src/effector/__tests__/fork/fork.test.ts b/src/effector/__tests__/fork/fork.test.ts
index 52a96310..113bb62e 100644
--- a/src/effector/__tests__/fork/fork.test.ts
+++ b/src/effector/__tests__/fork/fork.test.ts
@@ -85,6 +85,51 @@ describe('units without sids support', () => {
     await allSettled(fooFx, {scope})
     expect(fn).toBeCalled()
   })
+  test('mixed sid and no-sid case should work for tests', async () => {
+    //@ts-expect-error
+    const $foo = createStore(0, {sid: null})
+    const $bar = $foo.map(x => x * 2)
+
+    const $sid = createStore(0, {sid: '$sid'})
+    const $sidBar = $sid.map(x => x * 2)
+
+    const scope = fork({
+      values: [
+        [$foo, 1],
+        [$sid, 2],
+      ],
+    })
+
+    expect(scope.getState($bar)).toBe(2)
+    expect(scope.getState($sidBar)).toBe(4)
+  })
+  test('edge-case: mixed sid and no-sid case should work for tests, even there are build-in sids in factory', async () => {
+    const libFactory = () => {
+      return createStore(0, {sid: '$sid'})
+    }
+
+    //@ts-expect-error
+    const $foo = createStore(0, {sid: null})
+    const $bar = $foo.map(x => x * 2)
+
+    const $sid = libFactory()
+    const $sidBar = $sid.map(x => x * 2)
+
+    const $sidOther = libFactory()
+    const $sidBarOther = $sidOther.map(x => x * 2)
+
+    const scope = fork({
+      values: [
+        [$foo, 1],
+        [$sid, 2],
+        [$sidOther, 4],
+      ],
+    })
+
+    expect(scope.getState($bar)).toBe(2)
+    expect(scope.getState($sidBar)).toBe(4)
+    expect(scope.getState($sidBarOther)).toBe(8)
+  })
 })
 describe('fork values support', () => {
   test('values as js Map', async () => {
diff --git a/src/effector/__tests__/fork/index.test.ts b/src/effector/__tests__/fork/index.test.ts
index edaaa8f5..d46c9adb 100644
--- a/src/effector/__tests__/fork/index.test.ts
+++ b/src/effector/__tests__/fork/index.test.ts
@@ -648,19 +648,6 @@ describe('getState with same sids', () => {
 
     expect(scope.getState($convenientValue)).toEqual(false)
   })
-  test('sending values to stores with same sid is an error', async () => {
-    const a = createStore(0, {sid: 'sameSid'})
-    const b = createStore(0, {sid: 'sameSid'})
-
-    expect(() => {
-      fork({
-        values: [
-          [a, 1],
-          [b, 1],
-        ],
-      })
-    }).toThrowErrorMatchingInlineSnapshot(`"duplicate sid found"`)
-  })
 })
 
 describe('diamond deps (issue #613)', () => {
diff --git a/src/effector/__tests__/fork/serialize.test.ts b/src/effector/__tests__/fork/serialize.test.ts
index 3ab36d9c..cc883532 100644
--- a/src/effector/__tests__/fork/serialize.test.ts
+++ b/src/effector/__tests__/fork/serialize.test.ts
@@ -77,7 +77,7 @@ test('serialize: ignore with fork(values)', async () => {
   $b.on(inc, x => x + 1)
 
   const scope = fork({
-    values: [[$a, 100]]
+    values: [[$a, 100]],
   })
 
   await allSettled(inc, {scope})
@@ -621,6 +621,21 @@ describe('serialize: missing sids', () => {
       'There is a store without sid in this scope, its value is omitted',
     )
   })
+  test('serialize: throws if duplicated sids', () => {
+    const a = createStore(0, {sid: 'sameSid'})
+    const b = createStore(0, {sid: 'sameSid'})
+
+    const scope = fork({
+      values: [
+        [a, 1],
+        [b, 1],
+      ],
+    })
+
+    expect(() => serialize(scope)).toThrowErrorMatchingInlineSnapshot(
+      `"duplicate sid found in this scope"`,
+    )
+  })
   test('serialize: doesn not warn, if no sid is missing', () => {
     const $store = createStore('value')