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

improve merging of resolve and parsing options #9126

Merged
merged 1 commit into from
May 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/NormalModuleFactory.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const ModuleFactory = require("./ModuleFactory");
const NormalModule = require("./NormalModule");
const RawModule = require("./RawModule");
const RuleSet = require("./RuleSet");
const cachedMerge = require("./util/cachedMerge");
const { cachedCleverMerge } = require("./util/cleverMerge");

/** @typedef {import("./ModuleFactory").ModuleFactoryCreateData} ModuleFactoryCreateData */
/** @typedef {import("./ModuleFactory").ModuleFactoryResult} ModuleFactoryResult */
Expand Down Expand Up @@ -335,7 +335,7 @@ class NormalModuleFactory extends ModuleFactory {
typeof settings[r.type] === "object" &&
settings[r.type] !== null
) {
settings[r.type] = cachedMerge(settings[r.type], r.value);
settings[r.type] = cachedCleverMerge(settings[r.type], r.value);
} else {
settings[r.type] = r.value;
}
Expand Down
3 changes: 2 additions & 1 deletion lib/ResolverFactory.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

const Factory = require("enhanced-resolve").ResolverFactory;
const { HookMap, SyncHook, SyncWaterfallHook } = require("tapable");
const { cachedCleverMerge } = require("./util/cleverMerge");

/** @typedef {import("enhanced-resolve").Resolver} Resolver */

Expand Down Expand Up @@ -77,7 +78,7 @@ module.exports = class ResolverFactory {
resolver.withOptions = options => {
const cacheEntry = childCache.get(options);
if (cacheEntry !== undefined) return cacheEntry;
const mergedOptions = Object.assign({}, originalResolveOptions, options);
const mergedOptions = cachedCleverMerge(originalResolveOptions, options);
const resolver = this.get(type, mergedOptions);
childCache.set(options, resolver);
return resolver;
Expand Down
11 changes: 5 additions & 6 deletions lib/WebpackOptionsApply.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ const DefaultStatsFactoryPlugin = require("./stats/DefaultStatsFactoryPlugin");
const DefaultStatsPresetPlugin = require("./stats/DefaultStatsPresetPlugin");
const DefaultStatsPrinterPlugin = require("./stats/DefaultStatsPrinterPlugin");

const { cachedCleverMerge } = require("./util/cleverMerge");

/** @typedef {import("../declarations/WebpackOptions").WebpackOptions} WebpackOptions */
/** @typedef {import("./Compiler")} Compiler */

Expand Down Expand Up @@ -566,8 +568,7 @@ class WebpackOptionsApply extends OptionsApply {
{
fileSystem: compiler.inputFileSystem
},
options.resolve,
resolveOptions
cachedCleverMerge(options.resolve, resolveOptions)
);
});
compiler.resolverFactory.hooks.resolveOptions
Expand All @@ -578,8 +579,7 @@ class WebpackOptionsApply extends OptionsApply {
fileSystem: compiler.inputFileSystem,
resolveToContext: true
},
options.resolve,
resolveOptions
cachedCleverMerge(options.resolve, resolveOptions)
);
});
compiler.resolverFactory.hooks.resolveOptions
Expand All @@ -589,8 +589,7 @@ class WebpackOptionsApply extends OptionsApply {
{
fileSystem: compiler.inputFileSystem
},
options.resolveLoader,
resolveOptions
cachedCleverMerge(options.resolveLoader, resolveOptions)
);
});
compiler.hooks.afterResolvers.call(compiler);
Expand Down
36 changes: 0 additions & 36 deletions lib/util/cachedMerge.js

This file was deleted.

88 changes: 88 additions & 0 deletions lib/util/cleverMerge.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
MIT License http://www.opensource.org/licenses/mit-license.php
Author Tobias Koppers @sokra
*/

"use strict";

const mergeCache = new WeakMap();

/**
* Merges two given objects and caches the result to avoid computation if same objects passed as arguments again.
* @example
* // performs cleverMerge(first, second), stores the result in WeakMap and returns result
* cachedCleverMerge({a: 1}, {a: 2})
* {a: 2}
* // when same arguments passed, gets the result from WeakMap and returns it.
* cachedCleverMerge({a: 1}, {a: 2})
* {a: 2}
* @param {object} first first object
* @param {object} second second object
* @returns {object} merged object of first and second object
*/
const cachedCleverMerge = (first, second) => {
let innerCache = mergeCache.get(first);
if (innerCache === undefined) {
innerCache = new WeakMap();
mergeCache.set(first, innerCache);
}
const prevMerge = innerCache.get(second);
if (prevMerge !== undefined) return prevMerge;
const newMerge = cleverMerge(first, second);
innerCache.set(second, newMerge);
return newMerge;
};

/**
* Merges two objects. Objects are deeply clever merged.
* Arrays might reference the old value with "..."
* @param {object} first first object
* @param {object} second second object
* @returns {object} merged object of first and second object
*/
const cleverMerge = (first, second) => {
const newObject = Object.assign({}, first);
for (const key of Object.keys(second)) {
if (!(key in newObject)) {
newObject[key] = second[key];
continue;
}
const secondValue = second[key];
Copy link

Choose a reason for hiding this comment

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

This assignment should be before the if statement above. Then replace second[key] in the body of the if with secondValue.

Copy link
Member

Choose a reason for hiding this comment

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

Can you open issue and describe problem?

if (typeof secondValue !== "object" || secondValue === null) {
newObject[key] = secondValue;
Copy link

Choose a reason for hiding this comment

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

There should probably be continue here. If secondValue is not an object or null, then it will not be an Array and firstValue will equal secondValue in the else portion of the if below resulting in reassignment of newObject[key] to secondValue. The assignment of newObject[key] = secondValue currently occurs 4 times in this code. A cleaner approach might be to either introduce a variable called newValue which is initially set to secondValue, modified if needed, and then newObject[key] assigned to newValue at the end of the loop, OR negate the above if statement, wrap it around the one below, move this assignment to the end of the loop and add a continue after all other places where newObject[key] is set.

}
if (Array.isArray(secondValue)) {
const firstValue = newObject[key];
Copy link

Choose a reason for hiding this comment

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

This line is duplicated in the else portion of this if statement. The firstValue variable should thus be declared before the if statement.

if (Array.isArray(firstValue)) {
const newArray = [];
for (const item of secondValue) {
if (item === "...") {
for (const item of firstValue) {
newArray.push(item);
}
} else {
newArray.push(item);
}
}
newObject[key] = newArray;
} else {
newObject[key] = secondValue;
Copy link

Choose a reason for hiding this comment

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

See above comment above.

}
} else {
const firstValue = newObject[key];
if (
typeof firstValue === "object" &&
firstValue !== null &&
!Array.isArray(firstValue)
) {
newObject[key] = cleverMerge(firstValue, secondValue);
} else {
newObject[key] = secondValue;
Copy link

Choose a reason for hiding this comment

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

See comment above.

}
}
}
return newObject;
};

exports.cachedCleverMerge = cachedCleverMerge;
exports.cleverMerge = cleverMerge;
7 changes: 5 additions & 2 deletions test/cases/loaders/resolve/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ it("should be possible to create resolver with different options", () => {
const result = require("./loader!");
expect(result).toEqual({
one: "index.js",
two: "index.xyz"
two: "index.xyz",
three: "index.js",
four: "index.xyz",
five: "index.js"
});
})
});
19 changes: 17 additions & 2 deletions test/cases/loaders/resolve/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,28 @@ module.exports = function() {
const resolve2 = this.getResolve({
extensions: [".xyz", ".js"]
});
const resolve3 = this.getResolve({
extensions: [".hee", "..."]
});
const resolve4 = this.getResolve({
extensions: [".xyz", "..."]
});
const resolve5 = this.getResolve({
extensions: ["...", ".xyz"]
});
return Promise.all([
resolve1(__dirname, "./index"),
resolve2(__dirname, "./index")
]).then(([one, two]) => {
resolve2(__dirname, "./index"),
resolve3(__dirname, "./index"),
resolve4(__dirname, "./index"),
resolve5(__dirname, "./index")
]).then(([one, two, three, four, five]) => {
return `module.exports = ${JSON.stringify({
one: path.basename(one),
two: path.basename(two),
three: path.basename(three),
four: path.basename(four),
five: path.basename(five)
})}`;
});
};
2 changes: 1 addition & 1 deletion test/configCases/rule-set/resolve-options/a.js
Original file line number Diff line number Diff line change
@@ -1 +1 @@
module.exports = require("./wrong");
module.exports = require("./wrong") + require("./normal") + require("./wrong2");
2 changes: 1 addition & 1 deletion test/configCases/rule-set/resolve-options/b.js
Original file line number Diff line number Diff line change
@@ -1 +1 @@
module.exports = require("./wrong");
module.exports = require("./wrong") + require("./normal") + require("./wrong2");
1 change: 1 addition & 0 deletions test/configCases/rule-set/resolve-options/c.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = require("./wrong") + require("./normal") + require("./wrong2");
6 changes: 4 additions & 2 deletions test/configCases/rule-set/resolve-options/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
it("should allow to set custom resolving rules", function() {
var a = require("./a");
expect(a).toBe("ok");
expect(a).toBe("ok-normal-ok2");
var b = require("./b");
expect(b).toBe("wrong");
expect(b).toBe("ok-normal-ok2-yes");
var c = require("./c");
expect(c).toBe("wrong-normal-ok2");
});
1 change: 1 addition & 0 deletions test/configCases/rule-set/resolve-options/normal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = "-normal-";
1 change: 1 addition & 0 deletions test/configCases/rule-set/resolve-options/ok.ok.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = "ok-ok";
1 change: 1 addition & 0 deletions test/configCases/rule-set/resolve-options/ok2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = "ok2";
1 change: 1 addition & 0 deletions test/configCases/rule-set/resolve-options/ok2.yes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = "ok2-yes";
23 changes: 22 additions & 1 deletion test/configCases/rule-set/resolve-options/webpack.config.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,33 @@
module.exports = {
resolve: {
alias: {
"./wrong2": "./ok2"
}
},
module: {
rules: [
{
test: require.resolve("./a"),
resolve: {
alias: {
"./wrong": "./ok"
}
},
extensions: [".js", ".ok.js"]
}
},
{
test: require.resolve("./b"),
resolve: {
alias: {
"./wrong": "./ok"
},
extensions: ["...", ".ok.js"]
}
},
{
test: require.resolve("./b"),
resolve: {
extensions: [".yes.js", "..."]
}
}
]
Expand Down
1 change: 1 addition & 0 deletions test/configCases/rule-set/resolve-options/wrong2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = "wrong2";