cerebral / overmind

Overmind - Frictionless state management
https://overmindjs.org
MIT License
1.58k stars 95 forks source link

[BUG] Bug in merge config #478

Closed Kuechlin closed 3 years ago

Kuechlin commented 3 years ago

I have found a bug in overmind/config merge.ts

Steps to reproduce

export const config = merge(
    {
        effects
    },
    namespaced({
        item
    })
)

What is expected?

a config should be created

What is actually happening?

$ node dist/app.js
D:\Source\overmind-issue\node_modules\overmind\lib\config\merge.js:12
            aggr[key] = copy(target[key] || {}, source[key]);
                                   ^

TypeError: Cannot read property 'item' of undefined
    at D:\Source\overmind-issue\node_modules\overmind\lib\config\merge.js:12:36
    at Array.reduce (<anonymous>)
    at copy (D:\Source\overmind-issue\node_modules\overmind\lib\config\merge.js:7:32)
    at D:\Source\overmind-issue\node_modules\overmind\lib\config\merge.js:55:20
    at Array.reduce (<anonymous>)
    at Object.merge (D:\Source\overmind-issue\node_modules\overmind\lib\config\merge.js:34:50)
    at Object.<anonymous> (D:\Source\overmind-issue\dist\store\index.js:29:27)
    at Module._compile (internal/modules/cjs/loader.js:1137:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1157:10)
    at Module.load (internal/modules/cjs/loader.js:985:32)
error Command failed with exit code 1.

Workaround

this workaround works for me

export const config = merge(
    namespaced({
        item
    }),
    {
        effects
    }
)

How can this be fixed?

i have looked into the source code and i think i found the bug in overmind/packages/node_modules/overmind/src/config/merge.ts line 12

copy(target[key] || {}, source[key]) is throwing the error because the null check is missing the case where target is undefined

so to fix it i would suggest to implement it something like this:

copy(target && target[key] ? target[key] : {}, source[key])
christianalfoni commented 3 years ago

@Kuechlin This is already fixed in next and released very soon 😄

Kuechlin commented 3 years ago

@christianalfoni Ok nice, thank you 👍. Should I close this issue or will you close it when next is released? This is the fist issue i created 😃

christianalfoni commented 3 years ago

You can close it 😄

If you want to test it you can install overmind with the @next tag.

npm install overmind@next overmind-react@next (or whatever view library you using)