feat: relative cache support #114

cs6cs6 · 2023-10-05T16:31:40Z

…velopers or into ci machines.

Summary

Related Issues

linux-foundation-easycla · 2023-10-05T16:31:43Z

The committers listed above are authorized under a signed CLA.

✅ login: cs6cs6 (5f1c068, 1594416, 4b483a9, bf15419, a09bbcf, 0b7542c, 0b5795c, 9ff6720, c4597a7, 88af1e7, 1da9c6b)

eslint-github-bot · 2023-10-05T16:31:44Z

Hi @cs6cs6!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
There should be a space following the initial tag and colon, for example 'feat: Message'.
The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

cs6cs6 · 2023-10-09T17:07:36Z

Hello. How do I draw attention to this PR? I believe I am only awaiting feedback.

nzakas

Thanks for putting this together and sorry for the delay -- we are a small team and have a lot to do.

Overall I think this looks good. I left some feedback throughout.

designs/2023-relative-cache/README.md

…oss developers or into ci machines. (#16493)

…che and the new cache would look like.

…based on feedback.

nzakas

This is looking good. Left comments throughout on areas we can clarify and clean up.

designs/2023-relative-cache/README.md

nzakas · 2023-10-20T15:46:41Z

designs/2023-relative-cache/README.md

+
+I originally planned the shareable-cache flag so that users of eslint could not be surprised by changed cache behavior, and would also not have to regenerate their cache 
+for a patch version change. But I discovered by testing that there is no way to NOT force people to regenerate their eslint cache with this change. That's because by adding 
+the 'shareable-cache' flag, it adds a property to the ConfigArray object. Even if it's set to false (the old behavior), the object's structure changes with a new property 


How is this changing the ConfigArray? This setting is at the CLI level, not at the config level, so there shouldn't be any changes to the ConfigArray.

The object itself, along with all of its properties (set or not) is rolled into the computed hash code that goes into the file. So, simply by adding a new property, the hash computed of the object changes, even if that property is deliberately set to create no change in behavior.

designs/2023-relative-cache/README.md

Accepting code review suggestion where backticks are added for readability. Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>

Per code review, adding backticks for readability. Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>

…Open Questions.

designs/2023-relative-cache/README.md

mdjermanovic · 2023-10-25T23:32:26Z

designs/2023-relative-cache/README.md

+- `lib/cli.js`: Add the `shareableCache` option, defaulted to false, in the `translateOptions `function. 
+
+### Changing cache file serialization
+- `lib/cli-engine/lint-result-cache.js`: Add the properties `cwd` and `shareableCache` to the `LintResultCache` class. `cwd` is a string, the current working directory. `shareableCache` is a boolean, the result of the user's passed in command line parameter. Use these values in two main places: 


I think we should clarify what we mean by the current working directory that will be passed to the LintResultCache class. Is it process.cwd(), or cwd that was passed to ESLint/FlatESLint constructor? In the latter case, is there a way to pass cwd to file-entry-cache or we could end up calculating paths differently?

Another question is does file-entry-cache support relative paths? All tests seem to be with absolute paths:

https://github.com/jaredwray/file-entry-cache/blob/master/test/specs/cache.js

It will. I am just going to push up the code review sometime today. I have a complete and working branch on this task, and most of your questions wuold be resolved by the code review/better answered looking at the code changes.

Don't forget to answer the question regarding what cwd is from the previous comment.

Is it process.cwd(), or cwd that was passed to ESLint/FlatESLint constructor?

When created in cli-engine.js, it's options.cwd. It is also referred to as 'cwd' in the code.
When created in flat-eslint.js, it's processedOptions.cwd.

Do you think that cwd would be a better base location to store relative paths?

Do you mean in all cases? If so, the answer is no. I think it makes the most sense to have it relative to the cache file when --cache-location is not used.

I'm asking because when --cache-location is not set, the cache file location should be already cwd. I think the advantage of keeping the paths relative to the cache file directory is that they don't change if the current directory changes, for example in a monorepo. Do you have a particular scenario in mind where storing paths relative to cwd would be advantageous?

Yeah, as @fasttime noticed, when --cache-location is not used, the cache file defaults to .eslintcache in cwd. So if the logic would be to use the location of the cache file when --cache-location is not used, and cwd when --cache-location is used, then it's basically the same as - always use cwd.

For a use case where a monorepo has a single eslint cache file that's checked-in and shared between developers, and developers work and run eslint in project subdirectories (using --cache-location with a path to the single cache file somewhere in the root), I believe the only solution would be that the paths are relative to the cache file directory.

For other use cases, e.g., when the cache file is generated in CI and shared between CI runs (I believe that was the original use case in eslint/eslint#16493?), I'm not sure, it depends on where and how the cache file will be stored and used. It might be good to get more input from people requesting this feature.

I think we should just make a decision and see what happens when people test with it. This PR has been open since October of last year, and it seems like this is the last thing to decide on, so I don't think there's value in drawing this out further when we have a binary decision to make.

So I'd like to propose that we say the paths are always relative to location of the .eslintcache file.

So I'd like to propose that we say the paths are always relative to location of the .eslintcache file.

I agree.

nzakas · 2023-12-08T15:49:49Z

@cs6cs6 there were still some outstanding requests for you to consider here. Are you still working on this?

nzakas · 2024-01-09T22:39:48Z

@cs6cs6 just checking back to see if you intend on finishing this up?

cs6cs6 · 2024-01-12T13:12:44Z

Hello, I apologize for the delay. I am working on this today.

nzakas · 2024-01-17T20:32:14Z

@bmish @mdjermanovic can you take another look at this?

cs6cs6 · 2024-01-31T14:04:06Z

Hello. I am happy to answer further questions or clarify things. My team solved our critical build speed issue by removing a lot of our more costly linter rules, and running prettier outside of eslint instead of inside. But I would still like to be able to commit this change if you are interested. If you are not interested in this change, please let me know.

nzakas · 2024-02-05T18:22:26Z

Ping @bmish @mdjermanovic

mdjermanovic · 2024-02-05T18:40:44Z

I left a new comment on this conversation: #114 (comment)

Rec0iL99 · 2024-02-15T02:26:32Z

Hey @jaredwray, could you please reformat your last #114 (comment).

jaredwray · 2024-02-17T20:18:19Z

Hey @jaredwray, could you please reformat your last #114 (comment).

Yep. Did above. Going to get tests around this over the next 72 hours and respond here on it.

fasttime · 2024-05-27T05:29:50Z

designs/2023-relative-cache/README.md

+
+The new flag, shareable-cache, passed in on the command line, will default to false. Unless people explicitly set it to true, then the old algorithms will
+be used. Users of the eslint cache will still need to regenerate their cache no matter how they set this flag, because the current implementation forces a cache rebuild if there are
+any configuration changes. Adding a new property counts as a change. 


Can you clarify the last sentence? Is a new property some field in the cache, in the config, or what exactly? Will removing a property or changing its value also count as a change?

Any new version of ESLint invalidates cache entries stored by a previous version, so I think this is not a concern.

https://github.com/eslint/eslint/blob/d2d83e045ad03f024d1679275708054d789ebe20/lib/cli-engine/lint-result-cache.js#L50

jaredwray · 2024-06-05T18:58:50Z

@nzakas and @mdjermanovic let me know if you need any other changes or if you want file-entry-cache to work a different way with an example.

nzakas · 2024-06-06T16:02:51Z

@mdjermanovic I think you were the last one to look at file-entry-cache, do you recall if your concerns were addressed?

jaredwray · 2024-06-06T16:18:30Z

@mdjermanovic I think you were the last one to look at file-entry-cache, do you recall if your concerns were addressed?

@mdjermanovic just let me know anything you want changed and I can do it.

mdjermanovic · 2024-06-06T18:14:55Z

@jaredwray thanks for the support! I believe we'll need some changes or new features in file-entry-cache to support this use case. I'll take another look at the latest version of file-entry-cache and come up with an initial proposal in a day or two.

mdjermanovic · 2024-06-06T18:32:05Z

designs/2023-relative-cache/README.md

+```js
+function hashOfConfigFor(config, cwd) {
+    if (!configHashCache.has(config)) {
+        // replace the full directory path in the config string to make the hash location-neutral


Since this feature will only be implemented for the new config system, I believe there is no need for this because there are no absolute paths in configs.

A place where we do have absolute paths is LintResult#filePath, so that's something we'll need to convert from absolute to relative and vice versa when cache entries are stored and retrieved.

A place where we do have absolute paths is LintResult#filePath, so that's something we'll need to convert from absolute to relative and vice versa when cache entries are stored and retrieved.

Or, we could omit LintResult#filePath property from cache entries and add it back when cache entries are retrieved.

mdjermanovic · 2024-06-09T08:48:25Z

I see that the latest file-entry-cache v9 resolves relative paths passed to getFileDescriptor() as relative to process.cwd() and stores file keys as absolute paths in the cache file, as if the absolute paths were passed to getFileDescriptor().

What does everyone think about the following solution to support relative paths?

We're currently initializing cache this way:

this.fileEntryCache = fileEntryCache.create(
    cacheFileLocation,
    void 0,
    useChecksum
);

We could pass another argument, relative (boolean):

this.fileEntryCache = fileEntryCache.create(
    cacheFileLocation,
    void 0,
    useChecksum,
    relative
);

When relative is true, file-entry-cache operates in "relative" mode:

File keys in the cache file are stored as relative paths (e.g., ./src/app.js or ../src/app.js), relative to the location of the cache file.
getFileDescriptor() would still be getting absolute paths, but when searching for cache entries, it would convert them to relative paths.

mdjermanovic · 2024-06-09T09:56:13Z

Here a stackblitz demonstration of what we'd like to achieve:

https://stackblitz.com/edit/stackblitz-starters-axsbxs?file=package.json

nzakas · 2024-06-10T15:11:00Z

I see that the latest file-entry-cache v9 resolves relative paths passed to getFileDescriptor() as relative to process.cwd() and stores file keys as absolute paths in the cache file, as if the absolute paths were passed to getFileDescriptor().

I think hardcoding of process.cwd() is problematic as API users might set cwd to something else. Would it be possible to pass in the cwd?

cs6cs6 changed the title ~~This feature is to create an eslintcache that can be shared across de…~~ feat: relative cache support Oct 5, 2023

eslint-github-bot bot added the feature label Oct 5, 2023

nzakas added the Initial Commenting This RFC is in the initial feedback stage label Oct 10, 2023

nzakas requested changes Oct 11, 2023

View reviewed changes

cs6cs6 force-pushed the feat/16493_relative_cache branch from 14475ae to 1bcafba Compare October 11, 2023 19:16

mdjermanovic reviewed Oct 12, 2023

View reviewed changes

designs/2023-relative-cache/README.md Outdated Show resolved Hide resolved

cs6cs6 added 6 commits October 18, 2023 13:31

feat: This feature is to create an eslintcache that can be shared acr…

5f1c068

…oss developers or into ci machines. (#16493)

Improving introductory text for clarity based on feedback.

1594416

Fixing typos

4b483a9

Adding sample cache information to the README to show what the old ca…

bf15419

…che and the new cache would look like.

Adding more information about a CLI Alternative to this code change, …

a09bbcf

…based on feedback.

Updating RFC with improved design for shareableCache.

0b7542c

cs6cs6 force-pushed the feat/16493_relative_cache branch from a05d10b to 0b7542c Compare October 18, 2023 13:32

cs6cs6 requested a review from nzakas October 19, 2023 19:34

nzakas reviewed Oct 20, 2023

View reviewed changes

cs6cs6 and others added 4 commits October 25, 2023 12:28

Accepting feedback comments.

0b5795c

Update designs/2023-relative-cache/README.md

9ff6720

Accepting code review suggestion where backticks are added for readability. Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>

Update designs/2023-relative-cache/README.md

c4597a7

Per code review, adding backticks for readability. Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>

Per code review comment, answering Help Needed item and moving it to …

88af1e7

…Open Questions.

bmish reviewed Oct 25, 2023

View reviewed changes

designs/2023-relative-cache/README.md Show resolved Hide resolved

bmish reviewed Oct 25, 2023

View reviewed changes

designs/2023-relative-cache/README.md Show resolved Hide resolved

mdjermanovic reviewed Oct 25, 2023

View reviewed changes

designs/2023-relative-cache/README.md Show resolved Hide resolved

mdjermanovic reviewed Oct 25, 2023

View reviewed changes

nzakas mentioned this pull request Nov 3, 2023

Change Request: Eslintcache relative eslint/eslint#16493

Open

1 task

Removing references to now defunct flat-eslint.js

1da9c6b

nzakas mentioned this pull request Apr 1, 2024

Change Request: (Consider moving .eslintcache to node_modules/.cache) eslint/eslint#18245

Closed

1 task

fasttime reviewed May 27, 2024

View reviewed changes

nzakas added the tsc waiting Issues that require feedback from a TSC member label Jun 3, 2024

mdjermanovic removed the tsc waiting Issues that require feedback from a TSC member label Jun 5, 2024

mdjermanovic reviewed Jun 6, 2024

View reviewed changes

feat: relative cache support #114

Are you sure you want to change the base?

feat: relative cache support #114

Conversation

cs6cs6 commented Oct 5, 2023

Summary

Related Issues

linux-foundation-easycla bot commented Oct 5, 2023 • edited

eslint-github-bot bot commented Oct 5, 2023

cs6cs6 commented Oct 9, 2023

nzakas left a comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fasttime Jun 5, 2024 • edited

Choose a reason for hiding this comment

mdjermanovic Jun 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas commented Dec 8, 2023

nzakas commented Jan 9, 2024

cs6cs6 commented Jan 12, 2024

nzakas commented Jan 17, 2024

cs6cs6 commented Jan 31, 2024

nzakas commented Feb 5, 2024

mdjermanovic commented Feb 5, 2024

Rec0iL99 commented Feb 15, 2024

jaredwray commented Feb 17, 2024

fasttime May 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredwray commented Jun 5, 2024

nzakas commented Jun 6, 2024

jaredwray commented Jun 6, 2024

mdjermanovic commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic commented Jun 9, 2024

mdjermanovic commented Jun 9, 2024

nzakas commented Jun 10, 2024

linux-foundation-easycla bot commented Oct 5, 2023 •

edited

fasttime Jun 5, 2024 •

edited

mdjermanovic Jun 5, 2024 •

edited

fasttime May 27, 2024 •

edited