-
Notifications
You must be signed in to change notification settings - Fork 233
Conversation
ping @edmorley |
Missing documentation in the README and types. Other than that LGTM. |
Add types and documentation for Rule.resolve
Personally I'd say it's fine, but I have no power here. Up to @edmorley to review and merge. |
@cyjake Hi! Thank you very much for opening this PR. My day to day work no longer involves front-end development, so I don't actually use webpack any more, other than via helping to maintain Neutrino and webpack-chain - as such contributing to these projects is best effort. I can entirely emphasise with opening a PR and having to wait for it to be reviewed/merged - my oldest still-open public PR is from 3 years ago. However I would like to politely suggest that pinging a PR 5 times in 21 days is too much, and in many cases might actually be counter-productive, since it might annoy maintainers, and mean they are less inclined to use their spare time to take a look. A gentler pinging schedule might be say T+2weeks, T+6weeks, T+3 months etc. Anyway, I'll take a look at this PR at some point today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think resolve
also needs to be added here:
Lines 122 to 127 in dd229b5
'include', | |
'exclude', | |
'use', | |
'rules', | |
'oneOf', | |
'test', |
Similar to how resolve
is handled in the existing Config.resolve
usage:
Lines 144 to 148 in dd229b5
merge(obj = {}, omit = []) { | |
const omissions = [ | |
'node', | |
'output', | |
'resolve', |
Could you also add some merge tests to catch this missing change in merge()
? eg Add to the objects here:
Lines 180 to 402 in 6bb22fc
test('merge empty', () => { | |
const rule = new Rule(); | |
const obj = { | |
enforce: 'pre', | |
test: /\.js$/, | |
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: { | |
loader: 'babel-loader', | |
options: { | |
presets: ['alpha'], | |
}, | |
}, | |
}, | |
}; | |
const instance = rule.merge(obj); | |
expect(instance).toBe(rule); | |
expect(rule.toConfig()).toStrictEqual({ | |
enforce: 'pre', | |
test: /\.js$/, | |
include: ['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'], | |
}, | |
}, | |
], | |
}); | |
}); | |
test('merge with values', () => { | |
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'], | |
}, | |
}, | |
}, | |
}); | |
expect(rule.toConfig()).toStrictEqual({ | |
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'], | |
}, | |
}, | |
], | |
}); | |
}); | |
test('merge with omit', () => { | |
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'], | |
}, | |
}, | |
}, | |
}, | |
['use', 'oneOf', 'rules'], | |
); | |
expect(rule.toConfig()).toStrictEqual({ | |
test: /\.jsx$/, | |
enforce: 'pre', | |
include: ['gamma', 'delta', 'alpha', 'beta'], | |
exclude: ['alpha', 'beta'], | |
use: [ | |
{ | |
loader: 'babel-loader', | |
options: { | |
presets: ['alpha'], | |
}, | |
}, | |
], | |
}); | |
}); |
7287fdb
to
2858caf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great - thank you :-)
Released in v6.5.0 :-) |
Configuring resolve at rule level is possible starting from webpack 4.36.1, which is quite useful if same alias name (such as
@
) were shared at different level of packages.