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.

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

Jump to bottom

improve build deps tracking (ESM, non-JS, better warnings) #12419

Merged
merged 3 commits into from Jan 13, 2021
Merged

improve build deps tracking (ESM, non-JS, better warnings) #12419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50844ea
add test case for ESM tracking
sokra Jan 13, 2021
c453624
store build deps correctly in cache
sokra Jan 13, 2021
89aabf7
add experimental ESM tracking based on es-module-lexer
sokra Jan 13, 2021
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
106 changes: 89 additions & 17 deletions lib/FileSystemInfo.js
View file
Open in desktop
Expand Up @@ -16,6 +16,8 @@ const makeSerializable = require("./util/makeSerializable");
/** @typedef {import("./logging/Logger").Logger} Logger */
/** @typedef {import("./util/fs").InputFileSystem} InputFileSystem */

const supportsEsm = +process.versions.modules >= 83;

const resolveContext = createResolver({
resolveToContext: true,
exportsFields: []
Expand Down Expand Up @@ -880,6 +882,8 @@ class FileSystemInfo {
this._cachedDeprecatedFileTimestamps = undefined;
this._cachedDeprecatedContextTimestamps = undefined;

this._warnAboutExperimentalEsmTracking = false;

this._statCreatedSnapshots = 0;
this._statTestedSnapshotsCached = 0;
this._statTestedSnapshotsNotCached = 0;
Expand Down Expand Up @@ -1211,7 +1215,12 @@ class FileSystemInfo {
break;
}
case RBDT_FILE_DEPENDENCIES: {
// TODO this probably doesn't work correctly with ESM dependencies
// Check for known files without dependencies
if (/\.json5?$|\.yarn-integrity$|yarn\.lock$|\.ya?ml/.test(path)) {
Copy link
Member

Choose a reason for hiding this comment

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

What about markdown?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's usually not imported in node.js so it doesn't matter that much, but I also thought about that and concluded that it's not totally safe to do since there are dialects like MDX that could have dependencies.

process.nextTick(callback);
break;
}
// Check commonjs cache for the module
/** @type {NodeModule} */
const module = require.cache[path];
if (module && Array.isArray(module.children)) {
Expand Down Expand Up @@ -1248,15 +1257,73 @@ class FileSystemInfo {
});
}
}
} else if (supportsEsm && /\.m?js$/.test(path)) {
if (!this._warnAboutExperimentalEsmTracking) {
this.logger.info(
"Node.js doesn't offer a (nice) way to introspect the ESM dependency graph yet.\n" +
"Until a full solution is available webpack uses an experimental ESM tracking based on parsing.\n" +
"As best effort webpack parses the ESM files to guess dependencies. But this can lead to expensive and incorrect tracking."
);
this._warnAboutExperimentalEsmTracking = true;
}
const lexer = require("es-module-lexer");
lexer.init.then(() => {
this.fs.readFile(path, (err, content) => {
if (err) return callback(err);
try {
const context = dirname(this.fs, path);
const source = content.toString();
const [imports] = lexer.parse(source);
for (const imp of imports) {
try {
let dependency;
if (imp.d === -1) {
// import ... from "..."
dependency = JSON.parse(
source.substring(imp.s - 1, imp.e + 1)
);
} else if (imp.d > -1) {
// import()
let expr = source.substring(imp.s, imp.e).trim();
if (expr[0] === "'")
expr = `"${expr
.slice(1, -1)
.replace(/"/g, '\\"')}"`;
dependency = JSON.parse(expr);
} else {
// e.g. import.meta
continue;
}
queue.push({
type: RBDT_RESOLVE_FILE,
context,
path: dependency
});
} catch (e) {
this.logger.warn(
`Parsing of ${path} for build dependencies failed at 'import(${source.substring(
imp.s,
imp.e
)})'.\n` +
"Build dependencies behind this expression are ignored and might cause incorrect cache invalidation."
);
this.logger.debug(e.stack);
}
}
} catch (e) {
this.logger.warn(
`Parsing of ${path} for build dependencies failed and all dependencies of this file are ignored, which might cause incorrect cache invalidation..`
);
this.logger.debug(e.stack);
}
process.nextTick(callback);
});
}, callback);
break;
} else {
// Unable to get dependencies from module system
// This may be because of an incomplete require.cache implementation like in jest
// Assume requires stay in directory and add the whole directory
const directory = dirname(this.fs, path);
queue.push({
type: RBDT_DIRECTORY,
path: directory
});
this.logger.log(
`Assuming ${path} has no dependencies as we were unable to assign it to any module system.`
);
}
process.nextTick(callback);
break;
Expand Down Expand Up @@ -1606,7 +1673,7 @@ class FileSystemInfo {
if (err) {
if (this.logger) {
this.logger.debug(
`Error snapshotting file timestamp hash combination of ${path}: ${err}`
`Error snapshotting file timestamp hash combination of ${path}: ${err.stack}`
);
}
jobError();
Expand Down Expand Up @@ -1634,7 +1701,7 @@ class FileSystemInfo {
if (err) {
if (this.logger) {
this.logger.debug(
`Error snapshotting file hash of ${path}: ${err}`
`Error snapshotting file hash of ${path}: ${err.stack}`
);
}
jobError();
Expand Down Expand Up @@ -1664,7 +1731,7 @@ class FileSystemInfo {
if (err) {
if (this.logger) {
this.logger.debug(
`Error snapshotting file timestamp of ${path}: ${err}`
`Error snapshotting file timestamp of ${path}: ${err.stack}`
);
}
jobError();
Expand Down Expand Up @@ -1700,7 +1767,7 @@ class FileSystemInfo {
if (err) {
if (this.logger) {
this.logger.debug(
`Error snapshotting context timestamp hash combination of ${path}: ${err}`
`Error snapshotting context timestamp hash combination of ${path}: ${err.stack}`
);
}
jobError();
Expand Down Expand Up @@ -1728,7 +1795,7 @@ class FileSystemInfo {
if (err) {
if (this.logger) {
this.logger.debug(
`Error snapshotting context hash of ${path}: ${err}`
`Error snapshotting context hash of ${path}: ${err.stack}`
);
}
jobError();
Expand Down Expand Up @@ -1758,7 +1825,7 @@ class FileSystemInfo {
if (err) {
if (this.logger) {
this.logger.debug(
`Error snapshotting context timestamp of ${path}: ${err}`
`Error snapshotting context timestamp of ${path}: ${err.stack}`
);
}
jobError();
Expand Down Expand Up @@ -1791,7 +1858,7 @@ class FileSystemInfo {
if (err) {
if (this.logger) {
this.logger.debug(
`Error snapshotting missing timestamp of ${path}: ${err}`
`Error snapshotting missing timestamp of ${path}: ${err.stack}`
);
}
jobError();
Expand All @@ -1818,7 +1885,7 @@ class FileSystemInfo {
if (err) {
if (this.logger) {
this.logger.debug(
`Error snapshotting managed item ${path}: ${err}`
`Error snapshotting managed item ${path}: ${err.stack}`
);
}
jobError();
Expand Down Expand Up @@ -2348,6 +2415,11 @@ class FileSystemInfo {
this._fileHashes.set(path, null);
return callback(null, null);
}
if (err.code === "ERR_FS_FILE_TOO_LARGE") {
this.logger.warn(`Ignoring ${path} for hashing as it's very large`);
this._fileHashes.set(path, "too large");
return callback(null, "too large");
}
return callback(err);
}

Expand Down
10 changes: 5 additions & 5 deletions lib/cache/PackFileCacheStrategy.js
View file
Open in desktop
Expand Up @@ -1072,18 +1072,18 @@ class PackFileCacheStrategy {
return promise.then(() => {
if (reportProgress) reportProgress(0.8, "serialize pack");
this.logger.time(`store pack`);
const updatedBuildDependencies = new Set(this.buildDependencies);
for (const dep of newBuildDependencies) {
updatedBuildDependencies.add(dep);
}
const content = new PackContainer(
pack,
this.version,
this.buildSnapshot,
this.buildDependencies,
updatedBuildDependencies,
this.resolveResults,
this.resolveBuildDependenciesSnapshot
);
// You might think this breaks all access to the existing pack
// which are still referenced, but serializing the pack memorizes
// all data in the pack and makes it no longer need the backing file
// So it's safe to replace the pack file
return this.fileSerializer
.serialize(content, {
filename: `${this.cacheLocation}/index.pack`,
Expand Down
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@
"browserslist": "^4.14.5",
"chrome-trace-event": "^1.0.2",
"enhanced-resolve": "^5.7.0",
"es-module-lexer": "^0.3.26",
"eslint-scope": "^5.1.1",
"events": "^3.2.0",
"glob-to-regexp": "^0.4.1",
Expand All @@ -37,6 +38,7 @@
"devDependencies": {
"@babel/core": "^7.11.1",
"@babel/preset-react": "^7.10.4",
"@types/es-module-lexer": "^0.3.0",
"@types/jest": "^26.0.15",
"@types/node": "^14.14.10",
"babel-loader": "^8.1.0",
Expand Down
54 changes: 50 additions & 4 deletions test/BuildDependencies.test.js
View file
Open in desktop
Expand Up @@ -24,7 +24,6 @@ const exec = (n, options = {}) => {
if (code === 0) {
if (!options.ignoreErrors && /<[ew]>/.test(stdout))
return reject(stdout);
console.log(stdout);
resolve(stdout);
} else {
reject(new Error(`Code ${code}: ${stdout}`));
Expand All @@ -34,6 +33,8 @@ const exec = (n, options = {}) => {
});
};

const supportsEsm = +process.versions.modules >= 83;

describe("BuildDependencies", () => {
beforeEach(done => {
rimraf(cacheDirectory, done);
Expand All @@ -57,6 +58,14 @@ describe("BuildDependencies", () => {
path.resolve(inputDirectory, "config-dependency.js"),
"module.exports = 0;"
);
fs.writeFileSync(
path.resolve(inputDirectory, "esm-dependency.js"),
"module.exports = 0;"
);
fs.writeFileSync(
path.resolve(inputDirectory, "esm-async-dependency.mjs"),
"export default 0;"
);
await exec("0", {
invalidBuildDepdencies: true,
buildTwice: true,
Expand All @@ -70,14 +79,18 @@ describe("BuildDependencies", () => {
path.resolve(inputDirectory, "config-dependency.js"),
"module.exports = 1;"
);
fs.writeFileSync(
path.resolve(inputDirectory, "esm-dependency.js"),
"module.exports = 1;"
);
await exec("1");
fs.writeFileSync(
path.resolve(inputDirectory, "loader-dependency.js"),
"module.exports = Date.now();"
);
const now1 = Date.now();
await exec("2");
await exec("3");
expect(await exec("2")).toMatch(/Captured build dependencies/);
expect(await exec("3")).not.toMatch(/Captured build dependencies/);
fs.writeFileSync(
path.resolve(inputDirectory, "config-dependency"),
"module.exports = Date.now();"
Expand All @@ -86,7 +99,22 @@ describe("BuildDependencies", () => {
await exec("4");
const now3 = Date.now();
await exec("5");
const results = Array.from({ length: 6 }).map((_, i) =>
let now4, now5;
if (supportsEsm) {
fs.writeFileSync(
path.resolve(inputDirectory, "esm-dependency.js"),
"module.exports = Date.now();"
);
now4 = Date.now();
await exec("6");
fs.writeFileSync(
path.resolve(inputDirectory, "esm-async-dependency.mjs"),
"export default Date.now();"
);
now5 = Date.now();
await exec("7");
}
const results = Array.from({ length: supportsEsm ? 8 : 6 }).map((_, i) =>
require(`./js/buildDeps/${i}/main.js`)
);
for (const r of results) {
Expand All @@ -96,26 +124,44 @@ describe("BuildDependencies", () => {
}
expect(results[0].loader).toBe(0);
expect(results[0].config).toBe(0);
if (supportsEsm) expect(results[0].esmConfig).toBe(0);
expect(results[0].uncached).toBe(0);
// 0 -> 1 should not cache at all because of invalid buildDeps
expect(results[1].loader).toBe(1);
expect(results[1].config).toBe(1);
expect(results[1].esmConfig).toBe(1);
expect(results[1].uncached).toBe(1);
// 1 -> 2 should be invalidated
expect(results[2].loader).toBeGreaterThan(now1);
expect(results[2].config).toBe(1);
expect(results[2].esmConfig).toBe(1);
expect(results[2].uncached).toBe(1);
// 2 -> 3 should stay cached
expect(results[3].loader).toBe(results[2].loader);
expect(results[3].config).toBe(1);
expect(results[3].esmConfig).toBe(1);
expect(results[3].uncached).toBe(1);
// 3 -> 4 should be invalidated
expect(results[4].loader).toBeGreaterThan(now2);
expect(results[4].config).toBeGreaterThan(now2);
expect(results[4].esmConfig).toBe(1);
expect(results[4].uncached).toBe(results[4].config);
// 4 -> 5 should stay cached, but uncacheable module still rebuilds
expect(results[5].loader).toBe(results[4].loader);
expect(results[5].config).toBe(results[4].config);
expect(results[5].uncached).toBeGreaterThan(now3);
if (supportsEsm) {
// 5 -> 6 should be invalidated
expect(results[6].loader).toBeGreaterThan(now4);
expect(results[6].config).toBeGreaterThan(now4);
expect(results[6].esmConfig).toBeGreaterThan(now4);
expect(results[6].uncached).toBeGreaterThan(now4);
// 6 -> 7 should be invalidated
expect(results[7].loader).toBeGreaterThan(now5);
expect(results[7].config).toBeGreaterThan(now5);
expect(results[7].esmConfig).toBeGreaterThan(now5);
expect(results[7].esmAsyncConfig).toBeGreaterThan(now5);
expect(results[7].uncached).toBeGreaterThan(now5);
}
}, 100000);
});
1 change: 1 addition & 0 deletions test/fixtures/buildDependencies/esm-cjs-dep.js
View file
Open in desktop
@@ -0,0 +1 @@
module.exports = require("../../js/buildDepsInput/esm-dependency");
1 change: 1 addition & 0 deletions test/fixtures/buildDependencies/esm-dep.mjs
View file
Open in desktop
@@ -0,0 +1 @@
export { default } from "./esm-cjs-dep.js";
5 changes: 5 additions & 0 deletions test/fixtures/buildDependencies/esm.mjs
View file
Open in desktop
@@ -0,0 +1,5 @@
export { default } from "./esm-dep.mjs";

export const asyncDep = (
await import("../../js/buildDepsInput/esm-async-dependency.mjs")
).default;
2 changes: 2 additions & 0 deletions test/fixtures/buildDependencies/index.js
View file
Open in desktop
Expand Up @@ -3,5 +3,7 @@
module.exports = {
loader: require("./loader!"),
config: VALUE,
esmConfig: VALUE2,
esmAsyncConfig: VALUE3,
uncached: require("./module")
};