fix(webpack-cli): to void defaultEntry override the webpack config entry

[Mistyyyy]

if not pass the entry option through command line, webpack-cli would look for defaultEntry
The defaultEntry would override the entry in webpack config files

ISSUES CLOSED: #1288

What kind of change does this PR introduce?
a bugFix

Did you add tests for your changes?
yes

If relevant, did you update the documentation?

no relevant

#1288

Does this PR introduce a breaking change?

Nothing

[webpack-bot]

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

[jamesgeorge007]

Please remove this line.

[Mistyyyy]

done

[ematipico]

I think the solution should be different. I left some feedback on how to tackle the problem. Let me know what you think

[webpack-bot]

@Mistyyyy Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ematipico Please review the new changes.

[Mistyyyy]

@ematipico

I had the same issue while working with the output property. Try to apply the same strategy. Strategies work for all type of entries

After diving into the source code of webpack-merge, I found strategies can't work for the primitives type. It can only work for Array、Object、Function type.

The core code of webpack-merge is here

For example

const source = {
  entry: 'foo'
}

const target = {
  entry: 'bar'
}

const strategy = {
  entry: 'append', // or replace, the default is append
}

const result = merge.strategy(strategy)(source, target);

No matter what strategies are applied, target.entry always override source.entry.

So, if config entry and defaultEntry all exist, the defaultEntry will always override config entry. because this.basicGroup.run execute later than this.configGroup.run

Additional, I can't figure out the effect of the strategies applied to output.filename and output.path, I try to comment out this line, all tests passed. because the value of output.path and output.filename are all primitives type, the strategies don't work.

this.strategy = {
   'output.filename': 'prepend',
   'output.path': 'prepend',
};

I have updated my code, please review that. Thanks.

[ematipico]

Hey @Mistyyyy, thank you very much for your extensive explanation!
If your findings are true, I now wonder if this is a webpack-merge bug of if it is intended by design.

If it is a bug, I think we should file an issue in webpack-merge repository. If it is intended, this is a a big problem.

When I was refactoring the code few weeks back, I was facing a similar issue with output.filename and using the strategies actually worked! So now I am actually confused. I am going to take a look at the code and see if we are good to land the PR (it looks good though, nice work here, thank you)

[ematipico]

@Mistyyyy I did a quick try and the strategy actually works... This is the test that I run. Let me know if I missed something.

Folder contents of the test

webpack.config.js

const { resolve } = require('path');

module.exports = {
    mode: 'development',
    entry: {
        index: './file.js',
    },
    output: {
        path: resolve('./dist'),
        filename: '[name].bundle.js',
    },
};

file.js

console.log('js');

custom-entry.test.js

'use strict';
const { stat, readFileSync } = require('fs');
const { resolve } = require('path');
const { run } = require('../../utils/test-utils');

describe('default entry and config entry all exist', () => {
    it('should use config entry if config entry existed', done => {
        const { stdout, stderr } = run(__dirname, ['-c', 'webpack.config.js'], false);

        expect(stderr).toBeFalsy();
        expect(stdout).toBeTruthy();
        stat(resolve(__dirname, './dist/index.bundle.js'), (err, stats) => {
            expect(err).toBeFalsy();
            expect(stats.isFile()).toBe(true);
            done();
        });
        const data = readFileSync(resolve(__dirname, './dist/index.bundle.js'), 'utf-8');
        expect(data).toContain('console.log');
    });
});

Changes applied

BaseGroup.js

constructor() {
+  this.strategy = {
+     'entry': 'prepend'
+  }
}

[Mistyyyy]

@ematipico Yeah, I think you should add some defaultEntry file which cli can reach. like ./index.js ./src/index.js, and test it again. the purpose is to test defaultEntry would override config entry and strategy takes effect or not.

[Mistyyyy]

@ematipico Yeah, I think you should add some defaultEntry file which cli can reach. like ./index.js ./src/index.js, and test it again. the purpose is to test defaultEntry would override config entry and strategy takes effect or not.

The project structure should look like this

├── src
│   ├── file.js
│   └── index.js
├── webpack.config.js

[ematipico]

Issue found. The problem is inside BasicGroup

resolveFlags should be something like this:

   resolveFlags() {
        const { args } = this;
        const { outputOptions, options } = this.opts;
        Object.keys(args).forEach(arg => {
            if (this.WEBPACK_OPTION_FLAGS.includes(arg)) {
                outputOptions[arg] = args[arg];
            }

            if (arg === 'entry') {
                if (args[arg]) {
                    options.entry = this.resolveFilePath(args[arg], 'index.js');

                } else {
                    options.entry = this.resolveFilePath(args.entry, 'index.js');
                }
            }
        });

        // TODO: to add once webpack bundle analyzer is installed, which is not at the moment
        // if (arg == 'analyze') {
        // const { BundleAnalyzerPlugin } = require('webpack-bundle-analyzer');
        // this.opts.options.plugins = [new BundleAnalyzerPlugin()];
        // }
        if (args.sourcemap) {
            options.devtool = args.sourcemap || 'eval';
            outputOptions.devtool = args.sourcemap;
        }

        if (outputOptions['dev']) {
            outputOptions['prod'] = undefined;
        }
    }

Your solution was correct but here I basically shifted the responsibility to BasicGroup. It checks of the argument name is "entry". After that it checks its value (this was the bug, it wasn't doing it and it was creating an entry point property every time) and if it has a value, a resolve path is called. I hope it makes sense

[Mistyyyy]

Yes, you are right. It's the BasicGroup's responsibility to get the right entry for the whole project. However, I can't get the this.compilerConfiguration which may contain the config entry In the instance of BasicGroup. It should be passed as a parameter when Initialize the BasicGroup instance.

and the resolveFlags should like

 // The configuration is the result of this.configuration after ConfigGroup executing
 const configuration = obj  
 if (arg === 'entry') {
     // if the boolean of args[arg] is truthy, the command entry exists
     // it should override the config entry
      if (args[arg]) {
          options.entry = this.resolveFilePath(args[arg], 'index.js');
      } else if (!configuration.entry) {
          // the config entry doesn't exist, use defaultEntry
          options.entry = this.resolveFilePath(args[arg], 'index.js');
     }
}

[ematipico]

I don't think you need the configuration object. I tested that code against your tests and they pass (plus all the current ones pass too)

[Mistyyyy]

if (arg === 'entry') {
     if (args[arg]) {
        options.entry = this.resolveFilePath(args[arg], 'index.js');
     } else {
          options.entry = this.resolveFilePath(args.entry, 'index.js');
      }
}

I think the code above has no difference with the code below, because the arg is always entry.

if (arg === 'entry') {
        options.entry = this.resolveFilePath(args[arg], 'index.js');
}

The reason for all tests passes maybe you don't clean the output directory which contain previous test output bundle.

I run this test failed.

    it('should use config entry if config entry existed', done => {
        const { stdout } = run(__dirname, ['-c', '../webpack.config.js'], false);
        const summary = extractSummary(stdout);
        const outputDir = 'entry-with-config/binary';
        expect(summary['Output Directory']).toContain(outputDir);

        expect(stdout).toContain('./a.js');
        stat(resolve(__dirname, './binary/index.bundle.js'), (err, stats) => {
            expect(err).toBeFalsy();
            expect(stats.isFile()).toBe(true);
            done();
        });
    });

// test Structure
├── a.js
├── entry-with-config
│   ├── entry-with-config.test.js
│   └── index.js
└── webpack.config.js

Why I need the configuration object? because the BasicGroup only executes once. How I know choose command entry or defaultEntry.

If has config entry, I shouldn't get the defaultEntry and get command entry if args[arg] exists.
If no config entry, I should get the defaultEntry or command entry. which depends on if args[arg] exists.

In addition, I think Strategy is the best method to fix this issue if webpack-merge supports it.

[ematipico]

The reason for all tests passes maybe you don't clean the output directory which contain previous test output bundle

You might be right!!

And now that I think about the whole logic, I believe your solution, for now, it's the best option!

[ematipico]

@Mistyyyy I merged the PR too soon. Would you mind fixing a failing test? We got rid of the "extractSummary" bit. Sorry man, this was a mistake of mine!

[Mistyyyy]

@ematipico Ok, I will fix it.