Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

fix(chainedset): fix merge non-array #228

Closed
wants to merge 0 commits into from

Conversation

xiaoxiaojx
Copy link
Contributor

Rule.js

merge(obj, omit = []) {
if (!omit.includes('include') && 'include' in obj) {
this.include.merge(obj.include);
}

obj.include : RuleSetCondition

type RuleSetCondition =
| RegExp
| string
| ((value: string) => boolean)
| RuleSetConditions

when obj.include = RegExp or non-array , throw error ....

@edmorley
Copy link
Member

edmorley commented Jan 8, 2020

Hi! Do you have a use-case that this fixes? At first glance it seems like merge() is only meant to support an array?

@xiaoxiaojx
Copy link
Contributor Author

Hi! Do you have a use-case that this fixes? At first glance it seems like merge() is only meant to support an array?

.module.rule('rule-one')
.merge({
oneOf: [
{
test: /.(js|jsx|mjs)$/,
include: paths.appSrc,
use: getBabelLoader({
useCache: true,
isAnalyzer: false,
useHotLoader: true
})
}

In this case, include is string, will throw error

@xiaoxiaojx
Copy link
Contributor Author

Because we rely on some third-party loader rules, we can't ask them to modify it to adapt to our technical transformation using webpack-chain

Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed reply, and thank you for the use-case - it makes sense.

Whilst fixing this in ChainedSet is probably the most concise fix, I'm not sure we should change ChainedSet, since:

  • it seems counter-intuitive to allow merging different data types like that
  • it's not guaranteed that every usage of ChainedSet is for a configuration item that webpack permits being both an array and a non-array data type

As such, I was thinking it might be best to fix here instead:

if (!omit.includes('include') && 'include' in obj) {
this.include.merge(obj.include);
}
if (!omit.includes('exclude') && 'exclude' in obj) {
this.exclude.merge(obj.exclude);
}

Also, could you add a test for this here:

webpack-chain/test/Rule.js

Lines 248 to 329 in 33c6ab1

test('merge with values', t => {
const rule = new Rule();
rule
.test(/\.js$/)
.post()
.include.add('gamma')
.add('delta')
.end()
.use('babel')
.loader('babel-loader')
.options({ presets: ['alpha'] });
rule.merge({
test: /\.jsx$/,
enforce: 'pre',
include: ['alpha', 'beta'],
exclude: ['alpha', 'beta'],
rules: {
minifier: {
resourceQuery: /minify/,
use: {
minifier: {
loader: 'minifier-loader',
},
},
},
},
oneOf: {
inline: {
resourceQuery: /inline/,
use: {
url: {
loader: 'url-loader',
},
},
},
},
use: {
babel: {
options: {
presets: ['beta'],
},
},
},
});
t.deepEqual(rule.toConfig(), {
test: /\.jsx$/,
enforce: 'pre',
include: ['gamma', 'delta', 'alpha', 'beta'],
exclude: ['alpha', 'beta'],
rules: [
{
resourceQuery: /minify/,
use: [
{
loader: 'minifier-loader',
},
],
},
],
oneOf: [
{
resourceQuery: /inline/,
use: [
{
loader: 'url-loader',
},
],
},
],
use: [
{
loader: 'babel-loader',
options: {
presets: ['alpha', 'beta'],
},
},
],
});
});

Many thanks :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

2 participants