natergj / excel4node

Node module to allow for easy Excel file creation
MIT License
1.38k stars 215 forks source link

Remove deepmerge dependency #249

Closed SferaDev closed 5 years ago

SferaDev commented 6 years ago

This partially reverts commit 995e0d1dae2e41fc6c4b04b98e5e8e00d3ae75e7.

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

See issue https://github.com/KyleAMathews/deepmerge/issues/87

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

EthanRBrown commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.