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

fix: new instance of FlatESLint should load latest config file version #16608

Merged
merged 3 commits into from Dec 29, 2022
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
38 changes: 37 additions & 1 deletion lib/eslint/flat-eslint.js
Expand Up @@ -93,6 +93,7 @@ const FLAT_CONFIG_FILENAME = "eslint.config.js";
const debug = require("debug")("eslint:flat-eslint");
const removedFormatters = new Set(["table", "codeframe"]);
const privateMembers = new WeakMap();
const importedConfigFileModificationTime = new Map();

/**
* It will calculate the error and warning count for collection of messages per file
Expand Down Expand Up @@ -281,7 +282,42 @@ async function loadFlatConfigFile(filePath) {

debug(`Config file URL is ${fileURL}`);

return (await import(fileURL)).default;
const mtime = (await fs.stat(filePath)).mtime.getTime();
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if all of this is strictly necessary. Can't we just append ?time=${new Date().getTime()} to the URL and forgo needing to check the actual modification time?

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 would work, but every time we call loadFlatConfigFile a new instance would be created and cached forever. I left a note on why this solution might be problematic here:

* Note that we should not overuse queries (e.g., by appending the current time
* to always reload the config file module) as that could cause memory leaks
* because entries are never removed from the import cache.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, totally missed that. Makes sense. 👍


/*
* Append a query with the config file's modification time (`mtime`) in order
* to import the current version of the config file. Without the query, `import()` would
* cache the config file module by the pathname only, and then always return
* the same version (the one that was actual when the module was imported for the first time).
*
* This ensures that the config file module is loaded and executed again
* if it has been changed since the last time it was imported.
* If it hasn't been changed, `import()` will just return the cached version.
*
* Note that we should not overuse queries (e.g., by appending the current time
* to always reload the config file module) as that could cause memory leaks
* because entries are never removed from the import cache.
*/
fileURL.searchParams.append("mtime", mtime);

/*
* With queries, we can bypass the import cache. However, when import-ing a CJS module,
* Node.js uses the require infrastructure under the hood. That includes the require cache,
* which caches the config file module by its file path (queries have no effect).
* Therefore, we also need to clear the require cache before importing the config file module.
* In order to get the same behavior with ESM and CJS config files, in particular - to reload
* the config file only if it has been changed, we track file modification times and clear
* the require cache only if the file has been changed.
*/
if (importedConfigFileModificationTime.get(filePath) !== mtime) {
delete require.cache[filePath];
}

const config = (await import(fileURL)).default;

importedConfigFileModificationTime.set(filePath, mtime);

return config;
}

/**
Expand Down
80 changes: 80 additions & 0 deletions tests/lib/eslint/flat-eslint.js
Expand Up @@ -11,6 +11,7 @@
//------------------------------------------------------------------------------

const assert = require("assert");
const util = require("util");
const fs = require("fs");
const fsp = fs.promises;
const os = require("os");
Expand Down Expand Up @@ -41,6 +42,15 @@ function ensureDirectoryExists(dirPath) {
}
}

/**
* Does nothing for a given time.
* @param {number} time Time in ms.
* @returns {void}
*/
async function sleep(time) {
await util.promisify(setTimeout)(time);
}

//------------------------------------------------------------------------------
// Tests
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -5256,4 +5266,74 @@ describe("FlatESLint", () => {
});
});

describe("config file", () => {

it("new instance of FlatESLint should use the latest version of the config file (ESM)", async () => {
const cwd = path.join(getFixturePath(), `config_file_${Date.now()}`);
const configFileContent = "export default [{ rules: { semi: ['error', 'always'] } }];";
const teardown = createCustomTeardown({
cwd,
files: {
"package.json": '{ "type": "module" }',
"eslint.config.js": configFileContent,
"a.js": "foo\nbar;"
}
});

await teardown.prepare();

let eslint = new FlatESLint({ cwd });
let [{ messages }] = await eslint.lintFiles(["a.js"]);

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "semi");
assert.strictEqual(messages[0].messageId, "missingSemi");
assert.strictEqual(messages[0].line, 1);

await sleep(100);
await fsp.writeFile(path.join(cwd, "eslint.config.js"), configFileContent.replace("always", "never"));

eslint = new FlatESLint({ cwd });
[{ messages }] = await eslint.lintFiles(["a.js"]);

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "semi");
assert.strictEqual(messages[0].messageId, "extraSemi");
assert.strictEqual(messages[0].line, 2);
});

it("new instance of FlatESLint should use the latest version of the config file (CJS)", async () => {
const cwd = path.join(getFixturePath(), `config_file_${Date.now()}`);
const configFileContent = "module.exports = [{ rules: { semi: ['error', 'always'] } }];";
const teardown = createCustomTeardown({
cwd,
files: {
"eslint.config.js": configFileContent,
"a.js": "foo\nbar;"
}
});

await teardown.prepare();

let eslint = new FlatESLint({ cwd });
let [{ messages }] = await eslint.lintFiles(["a.js"]);

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "semi");
assert.strictEqual(messages[0].messageId, "missingSemi");
assert.strictEqual(messages[0].line, 1);

await sleep(100);
await fsp.writeFile(path.join(cwd, "eslint.config.js"), configFileContent.replace("always", "never"));

eslint = new FlatESLint({ cwd });
[{ messages }] = await eslint.lintFiles(["a.js"]);

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "semi");
assert.strictEqual(messages[0].messageId, "extraSemi");
assert.strictEqual(messages[0].line, 2);
});
});

});