-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
.aegir.js
Outdated
webpack: { | ||
resolve: { | ||
alias: { | ||
deepmerge$: path.resolve(__dirname, 'node_modules/deepmerge/dist/umd.js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need deepmerge
, can we replace it with lodash merge or defaultsDeep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for comment. Maybe we can replace deepmerge
(Or, Object.assign()
?) . I'll investigate that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.assign
does NOT do a deep merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to replace deepmerge
to lodash.merge
. It works and passed tests.
https://github.com/camelmasa/js-libp2p-keychain/commit/093711e21090e938e4f5fc50d12d1867c17e05fa
Should we replace the npm from deepmerge
to lodash.merge
? If that is okay, I'll rebase commits in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I rebased commits. If it looks good, Please merge it 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I updated deepmerge and libp2p-crypto npms for dependencies out of date.
Also, I added
.aegir.js
file for fixing deepmerge's issue. TehShrike/deepmerge#87We can choice to wait until fix the issue. Thought ?