chore: remove unnecessary tslib by targeting ES2021#11
Conversation
WalkthroughThis set of changes removes the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant checkAndPreparePackage (index.js)
participant installUsingNPM
participant downloadDirectlyFromNPM
participant fs/path modules
User->>checkAndPreparePackage: Call with package info
checkAndPreparePackage->>fs/path modules: Attempt to resolve native binding
alt Binding not found
checkAndPreparePackage->>installUsingNPM: Try install via npm
alt installUsingNPM fails
checkAndPreparePackage->>downloadDirectlyFromNPM: Download tarball directly
end
end
checkAndPreparePackage-->>User: Return result or error
Possibly related issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
eslint.config.mjsOops! Something went wrong! :( ESLint: 9.25.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.mjs lib/target.jsOops! Something went wrong! :( ESLint: 9.25.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.mjs lib/constants.jsOops! Something went wrong! :( ESLint: 9.25.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.mjs
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR removes the unnecessary tslib dependency by converting module imports and refactoring the code to target ES2021. Key changes include:
- Updating import statements in multiple files (e.g. src/index.ts, src/helpers.ts, lib/constants.js) to use native Node.js module syntax.
- Replacing tslib‑based helper functions with modern JavaScript features like nullish coalescing.
- Minor refactoring in asynchronous functions and error message formatting for consistency.
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Updated import style for Node.js built-ins to use namespace imports. |
| src/helpers.ts | Changed import and type annotation for fs to use namespace pattern. |
| src/constants.ts | Adjusted import for path to use namespace imports. |
| lib/target.js | Refactored to use nullish coalescing operators for clarity. |
| lib/index.js | Removed tslib dependency and updated async/await patterns accordingly. |
| lib/helpers.js | Converted tslib‑based imports to native require statements. |
| lib/constants.js | Updated import for path and removed tslib usage. |
| eslint.config.mjs | Disabled the unicorn/import-style rule to accommodate updated syntax. |
Files not reviewed (1)
- package.json: Language not supported
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
=======================================
Coverage 39.56% 39.56%
=======================================
Files 6 6
Lines 460 460
Branches 53 53
=======================================
Hits 182 182
Misses 278 278 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
size-limit report 📦
|
commit: |
📊 Package size report -4.74%↓
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
|
Deploy preview for napi-postinstall ready! ✅ Preview Built with commit 4557d5d. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
lib/index.js (1)
12-28: 🛠️ Refactor suggestionGuard against infinite or excessive HTTP redirects
fetch()recursively follows301/302responses but does not cap redirect depth or handle status codes such as 303/307/308.
A malicious registry endpoint could create an endless chain and exhaust the call stack.Consider adding a
maxRedirectsparameter:-function fetch(url) { +function fetch(url, maxRedirects = 8) { return new Promise((resolve, reject) => { https .get(url, res => { - if ((res.statusCode === 301 || res.statusCode === 302) && - res.headers.location) { - fetch(res.headers.location).then(resolve, reject); + if ([301, 302, 303, 307, 308].includes(res.statusCode) && res.headers.location) { + if (maxRedirects === 0) { + return reject(new Error('Too many HTTP redirects')); + } + fetch(res.headers.location, maxRedirects - 1).then(resolve, reject); return; }
🧹 Nitpick comments (3)
lib/helpers.js (2)
26-44: Replace deprecatedfs.rmdirSyncwithfs.rmSync
fs.rmdirSync(without therecursiveflag) has been deprecated since Node 14 and removed in Node 18.
Using the modern counterpart ensures forward‑compatibility:- fs.rmdirSync(dir); + // `force` handles readonly files, `recursive` removes children. + fs.rmSync(dir, { recursive: true, force: true });
98-103: Tiny optimisation: avoid reading the entirelddbinary
fs.readFileSync('/usr/bin/ldd', 'utf8')loads the whole executable into memory just to check for the string “musl”.
Usingfs.readFileSyncwithout encoding and scanning only the first few KB, or runningldd --version, would be lighter. Not critical, but worth considering for constrained environments.lib/index.js (1)
88-97: Ensure target directory exists beforefs.writeFileSync
downloadedNodePath()returns a path within the host package’slibdirectory, but there is no guarantee that this directory exists on disk (e.g. if the package was just created).
Addingfs.mkdirSync(path.dirname(nodePath), { recursive: true })before the write makes the function more robust.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
lib/constants.js.mapis excluded by!**/*.maplib/helpers.js.mapis excluded by!**/*.maplib/index.js.mapis excluded by!**/*.maplib/target.js.mapis excluded by!**/*.mappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
eslint.config.mjs(1 hunks)lib/constants.js(1 hunks)lib/helpers.js(5 hunks)lib/index.js(3 hunks)lib/target.js(1 hunks)package.json(0 hunks)src/constants.ts(1 hunks)src/helpers.ts(2 hunks)src/index.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (3)
lib/constants.js (3)
lib/index.js (1)
path(7-7)lib/helpers.js (1)
path(12-12)src/constants.ts (1)
require(7-9)
src/helpers.ts (1)
lib/helpers.js (2)
stats(29-29)fs(11-11)
lib/helpers.js (1)
src/helpers.ts (3)
removeRecursive(20-36)downloadedNodePath(38-41)getNapiInfoFromPackageJson(51-105)
🪛 GitHub Check: codecov/patch
src/helpers.ts
[warning] 23-23: src/helpers.ts#L23
Added line #L23 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
🔇 Additional comments (11)
eslint.config.mjs (1)
13-13: Good change for supporting the new import styleDisabling the
unicorn/import-stylerule allows for consistent use of namespace imports (import * as fs) throughout the codebase, aligning with the updated TypeScript configuration that disables synthetic default imports.tsconfig.json (2)
4-5: Well-executed TypeScript configuration updateSetting both
allowSyntheticDefaultImportsandesModuleInteroptofalsecorrectly enforces the use of namespace imports for CommonJS modules, which aligns with the goal of removingtslibdependency and using more accurate import patterns.
10-10: Good choice of targetSetting the target to ES2021 is appropriate as it natively supports features like async/await, nullish coalescing, and optional chaining, eliminating the need for TypeScript to generate helper functions via
tslib.src/constants.ts (1)
1-1: Correct import style updateChanging to namespace import syntax (
import * as path) is appropriate since Node.js core modules don't have a default export in the CommonJS module system. This aligns with the TypeScript configuration changes.src/index.ts (1)
4-7: Consistent import style changesSwitching to namespace imports for all Node.js core modules is the correct approach given the updated TypeScript configuration. These changes properly align with the goal of removing dependency on
tslibfor synthetic default imports.src/helpers.ts (2)
2-3: Appropriate namespace import conversionChanging to namespace imports for Node.js core modules aligns with the TypeScript configuration updates and ensures consistency across the codebase.
23-23: Correctly updated type referenceThe type annotation was properly updated to use the type from the namespace import (
fs.Statsinstead of the previously importedStatstype).The static analysis tool flagged this line as not covered by tests, but this is just a type annotation that has no runtime impact. No action needed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-23: src/helpers.ts#L23
Added line #L23 was not covered by testslib/target.js (1)
45-46: Nullish coalescing succinctly improves readability – looks goodSwitching from the previous verbose fallback logic to
??is correct and compatible with your ES 2021 target. No functional change is introduced, and the intent is clearer.lib/constants.js (1)
5-7: Directrequire("node:path")import and package.json resolution are fineThe simplified import pattern eliminates the need for
tslibhelpers and keeps the code consistent with other files. The synchronousrequire(path.resolve(...))call is also acceptable here because it runs exactly once at module load time.lib/helpers.js (1)
220-222: Nice concise fallback ingetErrorMessageUsing optional chaining keeps the helper minimal and robust.
lib/index.js (1)
104-107: Version consistency check is spot‑onGood call verifying that the “host” and binary packages share the same version number when
checkVersionis requested.
Summary by CodeRabbit
Refactor
Chores
tslibfrom the project.