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

Conversation

mdjermanovic
Copy link
Member

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Aims to fix #16576.

What changes did you make? (Give an overview)

This change appends query string with config file's mtime to its URL in order to force reload of the config file when it has been changed since the last load.

Is there anything you'd like reviewers to focus on?

Marked as draft as this approach doesn't seem to work with CJS config files.

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Dec 3, 2022
@netlify
Copy link

netlify bot commented Dec 3, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit b82bd56
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6398f0e332320a0008fc246b

@fasttime
Copy link
Member

fasttime commented Dec 5, 2022

I managed to get the CJS test passing by removing the config from the CommonJS module cache before the import. Basically:

index f041ac6a3..40ef95d39 100644
--- a/lib/eslint/flat-eslint.js
+++ b/lib/eslint/flat-eslint.js
@@ -284,6 +284,7 @@ async function loadFlatConfigFile(filePath) {

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

+    delete require.cache[filePath]; // Uncache CJS module if exists.
     return (await import(fileURL)).default;
 }

This keeps the ESM cache unchanged, so the module should only be reloaded when the URL changes because of the timestamp. Obviously, this is not a very clean solution. I'd wish there was a standard way to reload CJS or ESM modules without starting a new process.

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Dec 13, 2022
@mdjermanovic mdjermanovic marked this pull request as ready for review December 13, 2022 21:55
@mdjermanovic mdjermanovic requested a review from a team as a code owner December 13, 2022 21:55
@mdjermanovic
Copy link
Member Author

Seems to work with delete require.cache[filePath], @fasttime thanks for the suggestion!

@mdjermanovic
Copy link
Member Author

This keeps the ESM cache unchanged, so the module should only be reloaded when the URL changes because of the timestamp.

Oh, I overlooked this. Hmm, I think you're right, URL would already have a cache hit in the import cache, so deleting from the underlying require cache wouldn't cause reloading. Still, might be better to minimize side effects by deleting the entry only when it's necessary?

@fasttime
Copy link
Member

Yes, I can think of a few edge cases, although they don't seem very likely. For example, if a config file is required by some plugin after being manually deleted from the CJS module cache, that would trigger a reload. I don't know why a plugin or tool that uses ESLint would require a config file manually, but for such cases it would be safest to keep the config in the CJS module cache if it is not going to be reloaded.

It should be even possible to delete the config module from require.cache unconditionally before import, but restore it afterwards (probably in a finally block) if no new entry with the same key (filePath) was created. That would make a map of filePath to mtime superfluous.

@@ -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. 👍

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -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.

Ah, totally missed that. Makes sense. 👍

@nzakas
Copy link
Member

nzakas commented Dec 26, 2022

@mdjermanovic because it's been a couple of weeks, didn't want to merge without being sure this was considered done. If so, please feel free to merge.

@mdjermanovic mdjermanovic merged commit 87b2470 into main Dec 29, 2022
@mdjermanovic mdjermanovic deleted the issue16576 branch December 29, 2022 22:38
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jan 3, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.30.0` -> `8.31.0`](https://renovatebot.com/diffs/npm/eslint/8.30.0/8.31.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.31.0`](https://github.com/eslint/eslint/releases/tag/v8.31.0)

[Compare Source](eslint/eslint@v8.30.0...v8.31.0)

#### Features

-   [`52c7c73`](eslint/eslint@52c7c73) feat: check assignment patterns in no-underscore-dangle ([#&#8203;16693](eslint/eslint#16693)) (Milos Djermanovic)
-   [`b401cde`](eslint/eslint@b401cde) feat: add options to check destructuring in no-underscore-dangle ([#&#8203;16006](eslint/eslint#16006)) (Morten Kaltoft)
-   [`30d0daf`](eslint/eslint@30d0daf) feat: group properties with values in parentheses in `key-spacing` ([#&#8203;16677](eslint/eslint#16677)) (Francesco Trotta)

#### Bug Fixes

-   [`35439f1`](eslint/eslint@35439f1) fix: correct syntax error in `prefer-arrow-callback` autofix ([#&#8203;16722](eslint/eslint#16722)) (Francesco Trotta)
-   [`87b2470`](eslint/eslint@87b2470) fix: new instance of FlatESLint should load latest config file version ([#&#8203;16608](eslint/eslint#16608)) (Milos Djermanovic)

#### Documentation

-   [`4339dc4`](eslint/eslint@4339dc4) docs: Update README (GitHub Actions Bot)
-   [`4e4049c`](eslint/eslint@4e4049c) docs: optimize code block structure ([#&#8203;16669](eslint/eslint#16669)) (Sam Chen)
-   [`54a7ade`](eslint/eslint@54a7ade) docs: do not escape code blocks of formatters examples ([#&#8203;16719](eslint/eslint#16719)) (Sam Chen)
-   [`e5ecfef`](eslint/eslint@e5ecfef) docs: Add function call example for no-undefined ([#&#8203;16712](eslint/eslint#16712)) (Elliot Huffman)
-   [`a3262f0`](eslint/eslint@a3262f0) docs: Add mastodon link ([#&#8203;16638](eslint/eslint#16638)) (Amaresh  S M)
-   [`a14ccf9`](eslint/eslint@a14ccf9) docs: clarify files property ([#&#8203;16709](eslint/eslint#16709)) (Sam Chen)
-   [`3b29eb1`](eslint/eslint@3b29eb1) docs: fix npm link ([#&#8203;16710](eslint/eslint#16710)) (Abdullah Osama)
-   [`a638673`](eslint/eslint@a638673) docs: fix search bar focus on `Esc` ([#&#8203;16700](eslint/eslint#16700)) (Shanmughapriyan S)
-   [`f62b722`](eslint/eslint@f62b722) docs: country flag missing in windows ([#&#8203;16698](eslint/eslint#16698)) (Shanmughapriyan S)
-   [`4d27ec6`](eslint/eslint@4d27ec6) docs: display zh-hans in the docs language switcher ([#&#8203;16686](eslint/eslint#16686)) (Percy Ma)
-   [`8bda20e`](eslint/eslint@8bda20e) docs: remove manually maintained anchors ([#&#8203;16685](eslint/eslint#16685)) (Percy Ma)
-   [`b68440f`](eslint/eslint@b68440f) docs: User Guide Getting Started expansion ([#&#8203;16596](eslint/eslint#16596)) (Ben Perlmutter)

#### Chores

-   [`65d4e24`](eslint/eslint@65d4e24) chore: Upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).4.1 ([#&#8203;16729](eslint/eslint#16729)) (Brandon Mills)
-   [`8d93081`](eslint/eslint@8d93081) chore: fix CI failure ([#&#8203;16721](eslint/eslint#16721)) (Sam Chen)
-   [`8f17247`](eslint/eslint@8f17247) chore: Set up automatic updating of README ([#&#8203;16717](eslint/eslint#16717)) (Nicholas C. Zakas)
-   [`4cd87cb`](eslint/eslint@4cd87cb) ci: bump actions/stale from 6 to 7 ([#&#8203;16713](eslint/eslint#16713)) (dependabot\[bot])
-   [`fd20c75`](eslint/eslint@fd20c75) chore: sort package.json scripts in alphabetical order ([#&#8203;16705](eslint/eslint#16705)) (Darius Dzien)
-   [`10a5c78`](eslint/eslint@10a5c78) chore: update ignore patterns in `eslint.config.js` ([#&#8203;16678](eslint/eslint#16678)) (Milos Djermanovic)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43Ni4yIiwidXBkYXRlZEluVmVyIjoiMzQuNzYuMiJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1697
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 28, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Request: [flat config] re-read the eslint.config.js file if it has been changed
3 participants