feat: relative cache support #114
Conversation
|
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.
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
changed the title
|
Hello. How do I draw attention to this PR? I believe I am only awaiting feedback. |
cs6cs6
force-pushed
the
feat/16493_relative_cache
branch
from
14475ae
to
1bcafba
Compare
…oss developers or into ci machines. (#16493)
…che and the new cache would look like.
…based on feedback.
cs6cs6
force-pushed
the
feat/16493_relative_cache
branch
from
a05d10b
to
0b7542c
Compare
|
|
||
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Accepting code review suggestion where backticks are added for readability. Co-authored-by: Nicholas C. Zakas <[email protected]>
Per code review, adding backticks for readability. Co-authored-by: Nicholas C. Zakas <[email protected]>
| - `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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Don't forget to answer the question regarding what cwd is from the previous comment.
There was a problem hiding this comment.
Is it
process.cwd(), orcwdthat 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.
There was a problem hiding this comment.
Ok. I have updated the code, and the flat-eslint.js file no longer exists in my code branch. I see a lot of the code duplication around parameter parsing has been removed. I am updating my RFC accordingly.
In ESLint 9, eslint.js has been renamed to legacy-eslint.js and flat-eslint.js is now named eslint.js.
There was a problem hiding this comment.
@jaredwray are you still interested in submitting a pull request? Just let us know if you need any help.
There was a problem hiding this comment.
@fasttime - I made the changes to support the relative path and wanted to confirm that is the change you need to make it work with file-entry-cache.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks! Now it would be great if someone could draft a PR in ESLint to test these changes.
|
@cs6cs6 there were still some outstanding requests for you to consider here. Are you still working on this? |
|
@cs6cs6 just checking back to see if you intend on finishing this up? |
|
Hello, I apologize for the delay. I am working on this today. |
|
@bmish @mdjermanovic can you take another look at this? |
|
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. |
|
Ping @bmish @mdjermanovic |
|
I left a new comment on this conversation: #114 (comment) |
|
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. |
…velopers or into ci machines.
Summary
Related Issues