Skip to content

optimize alias plugin for huge alias objects #438

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.

Sign up for GitHub

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

optimize alias plugin for huge alias objects #438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
534660a
optimize alias plugin for huge alias objects
VincentBailly Dec 10, 2024
e4ace48
linting and formating
VincentBailly Dec 10, 2024
2c5fcf3
fix failing unit test
VincentBailly Dec 10, 2024
24b3c60
add TODO comment
VincentBailly Dec 10, 2024
44a2603
add unit test to cover new scenario
VincentBailly Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
237 changes: 143 additions & 94 deletions lib/AliasPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,13 @@

"use strict";

const forEachBail = require("./forEachBail");
const { PathType, getType } = require("./util/path");

/** @typedef {import("./Resolver")} Resolver */
/** @typedef {import("./Resolver").ResolveRequest} ResolveRequest */
/** @typedef {import("./Resolver").ResolveStepHook} ResolveStepHook */
/** @typedef {string | Array<string> | false} Alias */
/** @typedef {{alias: Alias, name: string, onlyModule?: boolean}} AliasOption */
/** @typedef {{alias: Alias, name: string, onlyModule?: boolean, nameWithSlash?: string, absolutePath?: string}} AliasOption */

module.exports = class AliasPlugin {
/**
Expand All @@ -23,6 +22,9 @@
constructor(source, options, target) {
this.source = source;
this.options = Array.isArray(options) ? options : [options];
for (const item of this.options) {
item.nameWithSlash = item.name + "/";
Copy link
Member

Choose a reason for hiding this comment

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

Why we need it?

Copy link
Author

Choose a reason for hiding this comment

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

It's just a memoization so we don't do this concatenation on every resolution, the concatenation creates an object that is short lived, creating work for the garbage collector. By creating the concatenated string once, storing it and reusing it we save the memory overhead of creating many times the same temporary objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the aliasing object gets truly large, it'd be worth investing in adding a dependency on a prefix-tree implementation that can handle the optional * component in the path, and use that to optimize the matching logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid shape changes it would be best to have this.options consist of fresh objects that have all required properties.

}
this.target = target;
}

Expand All @@ -32,6 +34,7 @@
*/
apply(resolver) {
const target = resolver.ensureHook(this.target);

/**
* @param {string} maybeAbsolutePath path
* @returns {null|string} absolute path with slash ending
Expand All @@ -43,112 +46,158 @@
}
return null;
};

/**
* @param {string} path path
* @param {string} maybeSubPath sub path
* @param {AliasOption} item item
* @returns {boolean} true, if path is sub path
*/
const isSubPath = (path, maybeSubPath) => {
const absolutePath = getAbsolutePathWithSlashEnding(maybeSubPath);
if (!absolutePath) return false;
return path.startsWith(absolutePath);
const isSubPath = (path, item) => {
if (item.absolutePath === undefined) {
item.absolutePath = getAbsolutePathWithSlashEnding(item.name);
}

if (!item.absolutePath) return false;
return path.startsWith(item.absolutePath);
};

resolver
.getHook(this.source)
.tapAsync("AliasPlugin", (request, resolveContext, callback) => {
const innerRequest = request.request || request.path;
if (!innerRequest) return callback();
forEachBail(
this.options,
(item, callback) => {
/** @type {boolean} */
let shouldStop = false;
if (
innerRequest === item.name ||
(!item.onlyModule &&
(request.request
? innerRequest.startsWith(`${item.name}/`)
: isSubPath(innerRequest, item.name)))
) {
/** @type {string} */
const remainingRequest = innerRequest.slice(item.name.length);
/**
* @param {Alias} alias alias
* @param {(err?: null|Error, result?: null|ResolveRequest) => void} callback callback
* @returns {void}
*/
const resolveWithAlias = (alias, callback) => {
if (alias === false) {
/** @type {ResolveRequest} */
const ignoreObj = {
...request,
path: false
};
if (typeof resolveContext.yield === "function") {
resolveContext.yield(ignoreObj);
return callback(null, null);
}
return callback(null, ignoreObj);
}
if (
innerRequest !== alias &&
!innerRequest.startsWith(alias + "/")
) {
shouldStop = true;
const newRequestStr = alias + remainingRequest;
/** @type {ResolveRequest} */
const obj = {
...request,
request: newRequestStr,
fullySpecified: false
};
return resolver.doResolve(
target,
obj,
"aliased with mapping '" +
item.name +
"': '" +
alias +
"' to '" +
newRequestStr +
"'",
resolveContext,
(err, result) => {
if (err) return callback(err);
if (result) return callback(null, result);
return callback();
}
);
}
return callback();
};
/**
* @param {null|Error} [err] error
* @param {null|ResolveRequest} [result] result
* @returns {void}
*/
const stoppingCallback = (err, result) => {
if (err) return callback(err);
let i = 0;

let resolveCallback;

if (result) return callback(null, result);
// Don't allow other aliasing or raw request
if (shouldStop) return callback(null, null);
return callback();
};
if (Array.isArray(item.alias)) {
return forEachBail(
item.alias,
resolveWithAlias,
stoppingCallback
while (i < this.options.length) {
const item = this.options[i];
i++;
if (
innerRequest === item.name ||
(!item.onlyModule &&
(request.request
? innerRequest.startsWith(item.nameWithSlash)
: isSubPath(innerRequest, item)))
) {
/** @type {string} */
const remainingRequest =
innerRequest === item.name
? undefined
: innerRequest.slice(item.name.length);
/**
* @param {Alias} alias alias
* @param {(err?: null|Error, result?: null|ResolveRequest) => void} callback callback
* @returns {void}
*/
if (Array.isArray(item.alias)) {
for (const alias of item.alias) {

Check failure on line 94 in lib/AliasPlugin.js

View workflow job for this annotation

GitHub Actions / lint 

Replace `··` with `↹`
// TODO: maybe shouldBail should be set to true for the last object?

Check failure on line 95 in lib/AliasPlugin.js

View workflow job for this annotation

GitHub Actions / lint 

Replace `····` with `↹↹`
const done = processAlias(
alias,
request,
callback,
innerRequest,
remainingRequest,
resolver,
target,
item,
resolveContext,
resolveCallback,
false
);
} else {
return resolveWithAlias(item.alias, stoppingCallback);
if (done) {
return;
}
}
} else {
const done = processAlias(
item.alias,
request,
callback,
innerRequest,
remainingRequest,
resolver,
target,
item,
resolveContext,
resolveCallback
);
if (done) {
return;
}
}
return callback();
},
callback
);
}
}
// No match
callback();
});
}
};

function processAlias(
alias,
request,
callback,
innerRequest,
remainingRequest,
resolver,
target,
item,
resolveContext,
resolveCallback,
shouldBail = true
) {
if (alias === false) {
/** @type {ResolveRequest} */
const ignoreObj = {
...request,
path: false
};
if (typeof resolveContext.yield === "function") {
resolveContext.yield(ignoreObj);
callback(null, null);
return true;
}
callback(null, ignoreObj);
return true;
}
if (innerRequest !== alias && !innerRequest.startsWith(alias + "/")) {
const newRequestStr = remainingRequest ? alias + remainingRequest : alias;
/** @type {ResolveRequest} */
const obj = {
...request,
request: newRequestStr,
fullySpecified: false
};
let done = false;
if (!resolveCallback) {
resolveCallback = (err, result) => {
if (err || result) {
done = true;
if (err) return callback(err);
return callback(null, result);
}
// TODO: support also logic when this callback is called async
if (shouldBail) {
return callback(null, null);
}
};
}
resolver.doResolve(
target,
obj,
"aliased with mapping '" +
item.name +
"': '" +
alias +
"' to '" +
newRequestStr +
"'",
resolveContext,
resolveCallback
);
// the resolveCallback will take care of calling the callback
return shouldBail || done;
}
}
8 changes: 7 additions & 1 deletion test/fallback.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
const fileSystem = Volume.fromJSON(
{
"/a/index": "",
"/a/dir/index": "",
"/a/dir/index": "",

Check failure on line 11 in test/fallback.test.js

View workflow job for this annotation

GitHub Actions / lint 

Replace `··` with `↹`
"/a/second/index": "",

Check failure on line 12 in test/fallback.test.js

View workflow job for this annotation

GitHub Actions / lint 

Replace `··` with `↹`
"/recursive/index": "",
"/recursive/dir/index": "",
"/recursive/dir/file": "",
Expand Down Expand Up @@ -94,6 +95,11 @@
expect(resolver.resolveSync({}, "/", "multiAlias/anotherDir")).toBe(
"/e/anotherDir/index"
);
// The multiAlias works as expected when the matching alias is the last one
expect(resolver.resolveSync({}, "/", "multiAlias/second")).toBe(
"/a/second/index"
);

Check failure on line 101 in test/fallback.test.js

View workflow job for this annotation

GitHub Actions / lint 

Delete `⏎`

});
it("should log the correct info", done => {
const log = [];
Expand Down
2 changes: 2 additions & 0 deletions types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ declare interface AliasOption {
alias: Alias;
name: string;
onlyModule?: boolean;
nameWithSlash?: string;
absolutePath?: string;
}
type AliasOptionNewRequest = string | false | string[];
declare interface AliasOptions {
Expand Down
Loading