feat: support cjs and esm both #13
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support eggjs/egg#5257
WalkthroughThe changes enhance the project’s TypeScript and ESLint configuration, refine workflow settings, update dependencies, and improve the LRU cache implementation. Additionally, documentation and license updates ensure clarity and compliance. The project now uses ES module syntax, ensuring modern JavaScript standards are met. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LRUCache
participant CacheItem
User ->> LRUCache: new LRU(max)
activate LRUCache
User ->> LRUCache: set(key, value, { maxAge })
activate LRUCache
LRUCache ->> CacheItem: Create CacheItem
activate CacheItem
CacheItem ->> LRUCache: Return CacheItem
deactivate CacheItem
LRUCache ->> User: Acknowledge set
User ->> LRUCache: get(key)
activate LRUCache
LRUCache ->> CacheItem: Retrieve CacheItem
CacheItem ->> LRUCache: Check expiration
LRUCache ->> User: Return value
deactivate CacheItem
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@types/node@12.20.55, npm/egg-bin@1.11.1, npm/eslint-config-egg@6.0.0, npm/eslint@4.19.1, npm/typescript@4.9.5 |
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
|
@SocketSecurity ignore npm/egg-bin@6.10.0 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 64 124 +60
Branches 15 32 +17
=========================================
+ Hits 64 124 +60 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
test/ylru.test.ts (2)
Line range hint
139-153: Check consistency in expiration handling.The tests for
maxAgewith sleep intervals are intended to verify that items do not expire prematurely. However, the repeated assertions for non-expired values might be redundant. Consider simplifying these assertions to improve test clarity and efficiency.- assert(lru.get(1, { maxAge }) !== undefined); - assert(lru.get('k2', { maxAge }) !== undefined); - assert(lru.get('k3', { maxAge }) !== undefined); + // Assert that values are still retrievable shortly before expiration + assert(lru.get(1) !== undefined); + assert(lru.get('k2') !== undefined); + assert(lru.get('k3') !== undefined);
[!TIP]
Codebase VerificationVerify cache behavior consistency.
The implementation of the
LRUclass insrc/index.tsshows that the cache management logic deviates from typical LRU behavior. Instead of evicting the least recently used items, the cache moves items to a secondary cache (#_cache) and resets the primary cache (#cache) when the size limit is reached. This behavior does not align with the expected behavior of an LRU cache.
src/index.ts:
getmethod: Retrieves items from#cacheand#_cache, checks for expiration, and updates expiration times.setmethod: Inserts items into#cache, updates expiration times, and calls_update._updatemethod: Manages cache size, moves items to#_cache, and resets#cachewhen the size limit is reached.Please review and ensure the cache behavior aligns with the expected LRU cache functionality.
Analysis chain
Line range hint
80-95: Verify cache behavior consistency.The assertions on cache sizes seem to imply that the cache is not behaving as a typical LRU cache, where the size should reflect the number of items that have not expired or been evicted. Please verify that the cache implementation aligns with the expected behavior of an LRU cache.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the LRU cache behavior by checking the source code implementation of cache management. # Test: Search for cache management logic in the source code. Expect: Clear explanation or comments on cache behavior. rg --type typescript $'LRU' --glob src/**/*.tsLength of output: 82
Script:
#!/bin/bash # Description: Verify the LRU cache behavior by checking the source code implementation of cache management. # Test: Search for cache management logic in the source code. Expect: Clear explanation or comments on cache behavior. rg --type ts LRU --glob 'src/**/*.ts'Length of output: 282
Script:
#!/bin/bash # Description: Extract the implementation of the LRU class from the source code. # Test: Extract the full implementation of the LRU class. ast-grep --lang typescript --pattern $'class LRU { $$$ }' src/index.tsLength of output: 4651
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- .eslintrc (1 hunks)
- .github/workflows/nodejs.yml (2 hunks)
- .gitignore (1 hunks)
- LICENSE (1 hunks)
- README.md (4 hunks)
- package.json (1 hunks)
- src/index.ts (1 hunks)
- test/bench.cjs (2 hunks)
- test/fixtures/ts/check.ts (1 hunks)
- test/ylru.test.ts (10 hunks)
- tsconfig.json (1 hunks)
Files skipped from review due to trivial changes (6)
- .github/workflows/nodejs.yml
- .gitignore
- LICENSE
- test/bench.cjs
- test/fixtures/ts/check.ts
- tsconfig.json
Additional context used
LanguageTool
README.md
[grammar] ~21-~21: The word “checkout” is a noun. The verb is spelled with a space. (NOUN_VERB_CONFUSION)
Context: ...r, faster LRU cache algorithm.** Please checkout [algorithm](https://github.com/dominict...
[uncategorized] ~51-~51: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...alue | null -{Number} options.maxAge: update expire time when get, value will...
[grammar] ~51-~51: The conjunction “when” requires the past participle “got”, “gotten”. Or did you mean “you get”? (IF_VB_PCT)
Context: ...ptions.maxAge: update expire time when get, value will becomeundefinedafterm...
[uncategorized] ~57-~57: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ..., options]) -{Number} options.maxAge: value will becomeundefinedafter `ma...
[style] ~58-~58: The adverb ‘never’ is usually put between ‘will’ and ‘be’. (ADVERB_WORD_ORDER)
Context: ...axAgepass. IfmaxAge` not set, value will be never expired. Set the value for key. ### l...
[grammar] ~64-~64: The word ‘greater’ is a noun or an adjective. A verb is missing or misspelled, or maybe a comma is missing. (MD_JJ)
Context: ...trategy of ylru, thekeys' length may greater thanmax. ```ts const lru = new LRU(...
[uncategorized] ~118-~118: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...//github.com/xudafeng/git-contributor), auto updated atThu Mar 28 2024 11:52:18 GMT+0800....
Markdownlint
README.md
18-18: null (MD036, no-emphasis-as-heading)
Emphasis used instead of a heading
114-114: null (MD045, no-alt-text)
Images should have alternate text (alt text)
114-114: null (MD045, no-alt-text)
Images should have alternate text (alt text)
114-114: null (MD045, no-alt-text)
Images should have alternate text (alt text)
114-114: null (MD045, no-alt-text)
Images should have alternate text (alt text)
114-114: null (MD045, no-alt-text)
Images should have alternate text (alt text)
114-114: null (MD045, no-alt-text)
Images should have alternate text (alt text)
116-116: null (MD045, no-alt-text)
Images should have alternate text (alt text)
116-116: null (MD045, no-alt-text)
Images should have alternate text (alt text)
116-116: Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing leading pipe (MD055, table-pipe-style)
Table pipe style
116-116: Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing trailing pipe (MD055, table-pipe-style)
Table pipe style
116-116: Expected: 6; Actual: 2; Too few cells, row will be missing data (MD056, table-column-count)
Table column count
Additional comments not posted (15)
.eslintrc (1)
2-5: Approved ESLint Configuration UpdateThe update to the ESLint configuration properly extends additional TypeScript rules and enforces node prefix rules, aligning with the project's goals of enhancing TypeScript support and maintaining consistent coding standards.
package.json (6)
5-7: Approved Node Version Requirement UpdateThe updated Node version requirement to
>= 18.19.0is necessary to support the new features and breaking changes introduced in this PR.
14-29: Approved Dependency UpdatesThe updates to the
devDependenciesensure that the project uses the latest and potentially more secure versions of these packages.
33-39: Approved Script Updates and AdditionsThe addition of new scripts like
bench,ci, andprepublishOnlyenhances the project's automation and CI/CD capabilities, aligning with modern development practices.
50-63: Approved Updates to Module ExportsThe updates to the
exportsfield to support different module formats are crucial for the dual support of CommonJS and ECMAScript modules, aligning with the PR's objectives.
65-68: Approved Updates to Included FilesThe updates to the
filesfield to includedistandsrcensure that the necessary files are packaged, supporting the new structure of the project.
69-70: Approved Updates to Main and Types ConfigurationThe updates to the
mainandtypesfields to point to the new paths in thedistdirectory align with the changes in the project structure and support the new module formats.src/index.ts (2)
1-8: Approved Introduction of InterfacesThe introduction of the
YLRUOptionsandCacheIteminterfaces enhances typing and clarity in the LRU cache implementation, aligning with the project's enhanced TypeScript support.
10-124: Approved Class Definition and MethodsThe
LRUclass is well-defined with appropriate methods for managing the cache. The use of private fields and methods enhances encapsulation and aligns with modern JavaScript practices.README.md (2)
Line range hint
30-68: Approved Updated Import Statements and Usage ExamplesThe updated import statements and usage examples in the
README.mdreflect the new ECMAScript module syntax and the use of thenewkeyword, aligning with the PR's objectives of supporting both CommonJS and ECMAScript Modules.
Line range hint
45-89: Approved Updated API DocumentationThe updated API documentation, including the use of the
newkeyword, provides clarity on how to initialize and use the LRU object, aligning with the updated implementation and module format support.Tools
LanguageTool
[grammar] ~64-~64: The word ‘greater’ is a noun or an adjective. A verb is missing or misspelled, or maybe a comma is missing. (MD_JJ)
Context: ...trategy of ylru, thekeys' length may greater thanmax. ```ts const lru = new LRU(...test/ylru.test.ts (4)
1-3: Approved import changes for ESM compatibility.The imports have been updated to use ECMAScript module syntax, aligning with the PR objectives to support both CommonJS and ECMAScript modules.
46-71: Approved async function updates and assert adjustments.The test cases have been updated to asynchronous functions using
asyncandawaitfor handling sleep operations, which is a modern approach to handling asynchronous code in JavaScript. The use ofassert.equalfor checking cache sizes is appropriate for ensuring correctness in cache behavior.
Line range hint
117-126: Approved maxAge tests for cache expiration.The test cases for
maxAgeproperly simulate scenarios where cache items expire after a specified duration. This ensures that the LRU cache handles time-based expiration correctly, which is crucial for caches in production environments.
199-209: Approved tests for cache expiration handling.The tests effectively simulate scenarios where multiple items expire at different times, which is essential for validating the robustness of the LRU cache's expiration logic.
| it('can update expired to 0', async () => { | ||
| const lru = new LRU(10); | ||
| lru.set('foo', 'bar', { maxAge: 100 }); | ||
| lru.get('foo', { maxAge: 0 }); | ||
| yield sleep(200); | ||
| await sleep(200); | ||
| assert(lru.get('foo') === 'bar'); | ||
| }); | ||
|
|
||
| it('can update expired when item in _cache', function* () { | ||
| it('can update expired when item in _cache', async () => { | ||
| const lru = new LRU(2); | ||
| lru.set('foo1', 'bar'); | ||
| lru.set('foo2', 'bar', { maxAge: 100 }); | ||
| lru.get('foo1', { maxAge: 100 }); | ||
| yield sleep(50); | ||
| await sleep(50); | ||
| assert(lru.get('foo1') === 'bar'); | ||
| assert(lru.get('foo2', { maxAge: 0 }) === 'bar'); | ||
| yield sleep(120); | ||
| await sleep(120); |
There was a problem hiding this comment.
Clarify behavior when updating expired items.
The tests for updating expired cache items to zero maxAge and checking their availability afterwards are crucial for understanding cache behavior under dynamic conditions. However, the logic seems complex and might benefit from additional inline comments explaining the expected outcomes.
+ // Test updating expired items to zero maxAge and verify they are still retrievable
lru.set('foo', 'bar', { maxAge: 100 });
lru.get('foo', { maxAge: 0 });
await sleep(200);
assert(lru.get('foo') === 'bar');Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('can update expired to 0', async () => { | |
| const lru = new LRU(10); | |
| lru.set('foo', 'bar', { maxAge: 100 }); | |
| lru.get('foo', { maxAge: 0 }); | |
| yield sleep(200); | |
| await sleep(200); | |
| assert(lru.get('foo') === 'bar'); | |
| }); | |
| it('can update expired when item in _cache', function* () { | |
| it('can update expired when item in _cache', async () => { | |
| const lru = new LRU(2); | |
| lru.set('foo1', 'bar'); | |
| lru.set('foo2', 'bar', { maxAge: 100 }); | |
| lru.get('foo1', { maxAge: 100 }); | |
| yield sleep(50); | |
| await sleep(50); | |
| assert(lru.get('foo1') === 'bar'); | |
| assert(lru.get('foo2', { maxAge: 0 }) === 'bar'); | |
| yield sleep(120); | |
| await sleep(120); | |
| it('can update expired to 0', async () => { | |
| const lru = new LRU(10); | |
| // Test updating expired items to zero maxAge and verify they are still retrievable | |
| lru.set('foo', 'bar', { maxAge: 100 }); | |
| lru.get('foo', { maxAge: 0 }); | |
| await sleep(200); | |
| assert(lru.get('foo') === 'bar'); | |
| }); | |
| it('can update expired when item in _cache', async () => { | |
| const lru = new LRU(2); | |
| lru.set('foo1', 'bar'); | |
| lru.set('foo2', 'bar', { maxAge: 100 }); | |
| lru.get('foo1', { maxAge: 100 }); | |
| await sleep(50); | |
| assert(lru.get('foo1') === 'bar'); | |
| assert(lru.get('foo2', { maxAge: 0 }) === 'bar'); | |
| await sleep(120); |
[skip ci] ## [2.0.0](v1.4.0...v2.0.0) (2024-06-22) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support eggjs/egg#5257 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a Least Recently Used (LRU) cache implementation with enhanced functionalities. - **Bug Fixes** - Improved cache item expiration handling and test case accuracy. - **Documentation** - Updated `README` with new import syntax and method signatures. - LICENSE changed to MIT License. - **Chores** - Updated ESLint configuration, Node.js version in workflows, and `.gitignore`. - **Refactor** - Converted test cases to use async/await and updated module imports to ES module syntax. - **Dependencies** - Updated `devDependencies` and added new scripts in `package.json`. - **Build Configuration** - Updated `tsconfig.json` for stricter typing and ES2022 target. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both ([#13](#13)) ([73e07bf](73e07bf))
BREAKING CHANGE: drop Node.js < 18.19.0 support
eggjs/egg#5257
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
READMEwith new import syntax and method signatures.Chores
.gitignore.Refactor
Dependencies
devDependenciesand added new scripts inpackage.json.Build Configuration
tsconfig.jsonfor stricter typing and ES2022 target.