Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Remove clone-deep dependency in favor of native structuredClone #214

Merged
merged 1 commit into from
Jul 2, 2024
Merged

fix: Remove clone-deep dependency in favor of native structuredClone #214

merged 1 commit into from
Jul 2, 2024

Conversation

askoufis
Copy link
Contributor

@askoufis askoufis commented Jul 2, 2024

Given this package's supported node version is >=18, we can safely use the native structuredClone global instead of clone-deep, removing 5 dependencies in the process. I don't think it behaves any differently to clone-deep on plain objects, so this change shouldn't result in any unexpected config changes, and the tests seem to agree (they all pass).

Let me know if there's a good reason not to do this.

@bebraw bebraw merged commit 882294a into survivejs:develop Jul 2, 2024
2 checks passed
@bebraw
Copy link
Member

bebraw commented Jul 2, 2024

Thanks a lot. I actually never published 6.0.0 but I figured it was safe to publish since Node 20 is LTS already. I included your change in 6.0.0.

@askoufis askoufis deleted the replace-clone-deep branch July 2, 2024 07:09
@msev
Copy link

msev commented Jul 2, 2024

Hi,

I have an issue since 6.0.0.

When i run yarn build, i have the following error:

Failed to load '/.config/webpack/prod.config.js' config
[webpack-cli] DOMException [DataCloneError]: async function terserMinify(input, sourceMap, minimizerOptions, extractComments) {
  /**
   * @param {any} valu...<omitted>...
} could not be cloned.
    at new DOMException (node:internal/per_context/domexception:53:5)
    at _joinArrays (/node_modules/webpack-merge/dist/join-arrays.js:64:20)
    at /node_modules/webpack-merge/dist/merge-with.js:32:17
    at Array.forEach (<anonymous>)
    at mergeTo (/node_modules/webpack-merge/dist/merge-with.js:31:10)
    at /node_modules/webpack-merge/dist/merge-with.js:23:15
    at Array.forEach (<anonymous>)
    at mergeWith (/node_modules/webpack-merge/dist/merge-with.js:22:10)
    at mergeWithOptions (/node_modules/webpack-merge/dist/index.js:83:41)
    at merge (/node_modules/webpack-merge/dist/index.js:45:35)

Here's my webpack configuration:

const path = require('path');
const { merge } = require('webpack-merge');
const TerserPlugin = require('terser-webpack-plugin');

const commonConfig = require(path.resolve('./.config/webpack/common.config.js'));
const customConfig = require(path.resolve('./webpack.custom.config.js'));

module.exports = merge(commonConfig, customConfig, {
  mode: 'production',
  devtool: false,
  stats: {
    assets: true,
    assetsSpace: 100,
    entrypoints: true,
    modules: false
  },
  optimization: {
    minimize: true,
    minimizer: [
      new TerserPlugin({
        terserOptions: {
          format: {
            comments: false
          }
        },
        extractComments: false,
        parallel: true
      })
    ],
    splitChunks: {
      minSize: 0,
      cacheGroups: {
        defaultVendors: false,
        vendors: {
          test: /[\\/]node_modules[\\/]/,
          chunks: 'all',
          name(module, chunks, cacheGroupKey) {
            const allChunksNames = chunks.map((item) => item.name).join('@');
            return `${cacheGroupKey}@${allChunksNames}`;
          }
        }
      }
    }
  }
});

@bebraw
Copy link
Member

bebraw commented Jul 2, 2024

@msev Thanks for the heads up. I reverted the change so now it's using clone-deep again. I also added a note that it shouldn't be removed.

I imagine there's some subtle difference between structuredClone and clone-deep and there was some structure in your configuration structuredClone wasn't able to handle.

The fix is included in 6.0.1.

@msev
Copy link

msev commented Jul 2, 2024

Thanks for the quick fix @bebraw!

Sorry to have spoiled the party 😅

@bebraw
Copy link
Member

bebraw commented Jul 2, 2024

No worries, it was something the test suite didn't catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants