Skip to content
This repository has been archived by the owner on Jul 22, 2022. It is now read-only.

Remove deepmerge dependency #249

Closed
wants to merge 1 commit into from
Closed

Remove deepmerge dependency #249

wants to merge 1 commit into from

Conversation

SferaDev
Copy link

This partially reverts commit 995e0d1.

When using excel4node on the browser (such as React), deepmerge gives some trouble with webpack.

See issue TehShrike/deepmerge#87

This pull request replaces deepmerge with _.merge. I'm not sure if Issue 226 ( #229 ) needed deepmerge for any special reason.

This partially reverts commit 995e0d1.
@EthanRBrown
Copy link

We're affected by this too...excel4node can't be webpack bundled, which we do for AWS Lambda deployments. @SferaDev, looking over the test failures, it looks like lodash.merge is not working as a drop-in replacement for deepmerge...have you dropped work on this?

@EthanRBrown
Copy link

In case it's helpful, I was able to resolve the problem with Webpack configuration:

  resolve: {                                                                                                                                                                                                 
    alias: {                                                                                                                                                                                                 
      // fixes issues with deepmerge resolution within excel4node; see                                                                                                                                       
      // https://github.com/KyleAMathews/deepmerge/issues/87                                                                                                                                                 
      deepmerge$: pathUtils.resolve(__dirname, 'node_modules/deepmerge/dist/umd.js'),                                                                                                                        
    },                                                                                                                                                                                                       
  },                                                                                                                                                                                                         

@SferaDev
Copy link
Author

We're affected by this too...excel4node can't be webpack bundled, which we do for AWS Lambda deployments. @SferaDev, looking over the test failures, it looks like lodash.merge is not working as a drop-in replacement for deepmerge...have you dropped work on this?

Yes, I thought it was a possible replacement, but then found out it wasn't.

In case it's helpful, I was able to resolve the problem with Webpack configuration:

  resolve: {                                                                                                                                                                                                 
    alias: {                                                                                                                                                                                                 
      // fixes issues with deepmerge resolution within excel4node; see                                                                                                                                       
      // https://github.com/KyleAMathews/deepmerge/issues/87                                                                                                                                                 
      deepmerge$: pathUtils.resolve(__dirname, 'node_modules/deepmerge/dist/umd.js'),                                                                                                                        
    },                                                                                                                                                                                                       
  },                                                                                                                                                                                                         

I'll check this out, if it works I might close the PR and leave the efforts.

On another topic @EthanRBrown Do you have minify/uglify on your project? Since this library isn't fully compliant with latest webpack ES it fails when building a production build on our end. The only workaround I found was ejecting the project and disabling the minify plugin.

@SferaDev SferaDev closed this Oct 30, 2018
@SferaDev SferaDev deleted the deepmerge branch October 30, 2018 07:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants