Update all dependencies to latest versions and add createAxiosInstance option #275
Conversation
WalkthroughThe codebase was migrated from CommonJS to ES module syntax across all scripts, examples, and tests. The Node.js version requirement was updated to 22.0.0 or higher, and several dependencies were upgraded. Documentation and usage examples were updated to reflect ES module imports. Minor code refactoring and parameter renaming were performed for clarity and consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant UserScript as User Script (e.g., examples/first-steps.js)
participant Unifi as Unifi.Controller
participant Axios as Axios Instance
participant ControllerAPI as UniFi Controller API
UserScript->>Unifi: import Unifi from 'node-unifi'
UserScript->>Unifi: new Controller(opts)
UserScript->>Unifi: await controller.login()
Unifi->>Axios: POST /api/login
Axios->>ControllerAPI: HTTP request
ControllerAPI-->>Axios: HTTP response (session cookie)
Axios-->>Unifi: Response
Unifi-->>UserScript: Login result
UserScript->>Unifi: await controller.get5minSiteStats()
Unifi->>Axios: GET /api/s/{site}/stat/health
Axios->>ControllerAPI: HTTP request
ControllerAPI-->>Axios: HTTP response
Axios-->>Unifi: Response
Unifi-->>UserScript: Site stats
UserScript->>Unifi: await controller.logout()
Unifi->>Axios: POST /api/logout
Axios->>ControllerAPI: HTTP request
ControllerAPI-->>Axios: HTTP response
Axios-->>Unifi: Response
Unifi-->>UserScript: Logout result
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![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
🧹 Nitpick comments (16)
examples/custom-axios-client.js (1)
21-21: Consider adding a check for trustedCertificate.The code assumes
trustedCertificatewill be provided, but there's no validation if it's missing or invalid.- httpsAgent: new HttpsCookieAgent({cookies, requestCert: true, ca: trustedCertificate}), + httpsAgent: new HttpsCookieAgent({ + cookies, + requestCert: true, + ...(trustedCertificate ? { ca: trustedCertificate } : {}) + }),test/01-login-logout.js (1)
91-91: Check for negative array length might be incorrect.This condition checks if
result.length < 0, which is impossible for arrays as their length is always non-negative.- if ((result) === undefined || result.length < 0) { + if ((result) === undefined || result.length <= 0) {package.json (1)
29-35: Review updated development dependencies
Theeslint,mocha, andxoupgrades modernize linting and testing. Confirm that any new lint rules or CLI flags required by these versions are reflected in your configuration (e.g.,.eslintrcor test scripts).You may want to pin versions more strictly (no
latest) for reproducible CI runs.unifi.js (13)
45-53: Simplify default option assignments
The chained ternary checks for eachthis.optsproperty work, but they are verbose. Consider using destructuring with defaults (e.g.,const { host='unifi', port=8443, ... } = this.opts; this.opts = { host, port, ... }) to improve readability and reduce repetition.
238-240: Use object shorthand in payload construction
IncreateUser, you write:const new_user = { mac: mac.toLowerCase(), user_group_id, };You can destructure and use shorthand where possible to make this more concise.
310-323: DRY: Abstract common site-stats payload
The three site-level stats methods (get5minSiteStats,getHourlySiteStats,getDailySiteStats) share nearly identicalattrs,start, andendpayload construction. Extract a helper like:async _fetchSiteStats(path, attrs, start, end) { … }to avoid duplication.
Also applies to: 347-360, 384-397
463-469: DRY: Abstract common AP-stats payload
The AP stats methods repeat the same payload pattern. A shared helper (e.g.,_fetchApStats(path, attrs, start, end, mac)) would reduce boilerplate and improve maintainability.Also applies to: 498-504, 533-539, 571-577
613-624: DRY: Abstract common user-stats payload
The user stats methods defaultattribsthen build the payload similarly each time. Centralizing that into a function (e.g.,_fetchUserStats(path, mac, start, end, attribs)) will DRY up this section.Also applies to: 654-665, 697-708, 740-751
783-789: DRY: Abstract common gateway-stats payload
The gateway stats methods also follow the same pattern. A helper like_fetchGatewayStats(path, start, end, attribs)would eliminate the repeated code.Also applies to: 823-828, 863-868, 903-908
973-977: Consolidate parameterized query methods
Methods such asgetIPSEvents,getSessions, andgetLatestSessionsbuild payloads with_limit,_sort, etc. Consider a general-purpose pagination helper to standardize this logic across calls.Also applies to: 1003-1007, 1025-1028
1173-1176: Validate editClientName payload
The PUT to/rest/user/${id}with{ name }matches the API signature. Consider trimmingnamehere (name: name.trim()) to avoid sending accidental whitespace.
1201-1204: Standardize usergroup payload patterns
BothcreateUserGroupandeditUserGroupbuild similar payloads. You might extract a small helper to build{ _id?, name, qos_rate_max_down, qos_rate_max_up }instead of repeating the object literal.Also applies to: 1223-1228
1263-1266: Abstract AP group CRUD payload
createAPGroupandeditAPGroupshare a payload shape. Consider extracting{ device_macs, name, … }construction into a helper to reduce duplication.Also applies to: 1282-1287
1323-1327: Abstract firewall group CRUD payload
Similarly,createFirewallGroupandeditFirewallGroupboth build{ name, group_type, group_members, site_id? }. A small helper would improve readability.Also applies to: 1348-1353
2819-2823: Parameter rename clarity
The method signature switched fromport_idxtoport_index, but the payload still usesport_idx. Update the JSDoc to reflectport_indexand avoid confusion.
3248-3258: Unify API response error handling
The check ofbody.meta.rc === 'ok'and error construction is correct. You could centralize this logic into a helper to reduce branching complexity and improve testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.github/workflows/CI.yml(1 hunks)README.md(2 hunks)examples/README.md(1 hunks)examples/custom-axios-client.js(1 hunks)examples/first-steps.js(1 hunks)examples/listen-events.js(1 hunks)examples/poe-switch-onoff.js(1 hunks)package.json(3 hunks)test/01-login-logout.js(23 hunks)unifi.js(68 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
examples/first-steps.js (3)
examples/listen-events.js (6)
host(8-8)port(9-9)username(10-10)password(11-11)unifi(13-13)loginData(17-17)examples/poe-switch-onoff.js (7)
host(8-8)port(9-9)username(10-10)password(11-11)unifi(16-16)loginData(20-20)logoutData(49-49)unifi.js (1)
Unifi(3316-3316)
test/01-login-logout.js (1)
examples/first-steps.js (2)
sysinfo(25-25)sites(20-20)
examples/poe-switch-onoff.js (3)
examples/first-steps.js (7)
host(7-7)port(8-8)username(9-9)password(10-10)unifi(12-12)loginData(16-16)logoutData(40-40)examples/listen-events.js (6)
host(8-8)port(9-9)username(10-10)password(11-11)unifi(13-13)loginData(17-17)unifi.js (1)
Unifi(3316-3316)
examples/listen-events.js (3)
examples/first-steps.js (6)
host(7-7)port(8-8)username(9-9)password(10-10)unifi(12-12)loginData(16-16)examples/poe-switch-onoff.js (6)
host(8-8)port(9-9)username(10-10)password(11-11)unifi(16-16)loginData(20-20)unifi.js (1)
Unifi(3316-3316)
🔇 Additional comments (30)
.github/workflows/CI.yml (2)
28-28: Node.js version matrix has been appropriately updated.The CI workflow now tests against the latest maintained Node.js versions (20.x, 22.x, 23.x, 24.x), which aligns with the updated Node.js engine requirement in package.json. This is a good update to ensure compatibility with current Node.js versions.
33-72: Improved YAML formatting with consistent indentation.The indentation has been corrected from two spaces to six spaces for all steps under the job, improving readability and YAML syntax compliance.
examples/README.md (1)
7-7: Updated import statement to use ES module syntax.The import statement example has been correctly updated from CommonJS (
require('../unifi.js')) to ES module syntax (import Unifi from '../unifi.js'), which is consistent with the codebase migration to ES modules. This ensures users follow the proper import pattern when using the library.README.md (2)
40-40: Updated import statement to use ES module syntax.The example code has been correctly updated to use ES module syntax (
import Unifi from 'node-unifi';) instead of CommonJS (require('node-unifi')), which aligns with the overall migration to ES modules.
92-92: Updated import statement in EventEmitter example to use ES module syntax.The EventEmitter WebSockets example has been updated to use ES module import syntax, maintaining consistency with the rest of the codebase.
examples/first-steps.js (2)
3-5: Replaced CommonJS require with ES module imports.The script has been updated to use proper ES module imports for both the process module (using the node: prefix for Node.js built-in modules) and the Unifi module. This change aligns with the ES module migration in the rest of the codebase.
14-44: Refactored to use top-level await instead of IIFE.The script has been improved by replacing the immediately invoked async function expression (IIFE) with top-level await within a try-catch block. This simplifies the code structure while maintaining the same functionality and error handling. Top-level await is a feature available in ES modules that makes asynchronous code cleaner and more readable.
examples/custom-axios-client.js (2)
1-32: Well-implemented example for custom Axios client configuration.This new example effectively demonstrates the custom Axios instance feature mentioned in the PR objectives. It properly configures an HTTP client with cookie support and custom certificate handling.
18-23: Good implementation of the createAxiosInstance feature.The implementation shows how to properly customize the Axios instance with cookie support and certificate handling, which was a key objective of this PR.
test/01-login-logout.js (2)
2-6: ESM migration is properly implemented.The changes to import statements align with the project's migration to ES modules as mentioned in the PR objectives.
52-52: Consistent improvement to undefined checks.The changes from
typeof (variable) === 'undefined'to(variable) === undefinedimprove code readability while maintaining the same functionality.Also applies to: 64-64, 76-76, 91-91, 109-109, 120-120, 133-133, 146-146, 155-155, 169-169, 180-180, 194-194, 211-213, 231-231, 248-248, 273-273, 287-287, 301-301, 309-309, 317-317, 325-325, 333-333, 341-341, 349-349, 357-357, 365-365, 373-373, 381-381, 389-389, 397-397, 405-405, 413-413, 421-421, 429-429, 437-437, 445-445, 453-453, 461-461, 471-471, 479-479, 487-487, 507-507, 515-515, 523-523, 531-531, 539-539, 550-550, 558-558, 569-569
examples/listen-events.js (3)
3-5: ESM imports are properly implemented.The changes to import statements align with the project's migration to ES modules as mentioned in the PR objectives.
15-47: Refactoring to top-level await improves code readability.The refactoring from an IIFE to using top-level await with a try-catch block makes the code cleaner and more modern, aligning with current JavaScript best practices.
24-44: Well-preserved event handling logic.The event handling implementation remains intact during the refactoring, preserving the original functionality.
examples/poe-switch-onoff.js (4)
4-5: ESM imports are properly implemented.The changes to import statements align with the project's migration to ES modules as mentioned in the PR objectives.
13-13: Variable rename improves code clarity.Renaming
portIdxtoportIndexmakes the code more readable and self-documenting.
18-53: Refactoring to top-level await improves code structure.The refactoring from an IIFE to using top-level await with a try-catch block makes the code cleaner and more modern, aligning with current JavaScript best practices.
37-37: Consistent parameter naming.The change from
item.port_idxto checking againstportIndexmaintains consistent naming throughout the code.package.json (2)
8-8: ESM Migration: Set package type to module
Adding"type": "module"aligns with the ES module syntax in the codebase. Ensure that any scripts or tooling that expect CommonJS are updated accordingly.
21-26: Review updated runtime dependencies
Upgradingaxios,tough-cookie, andwsto their latest versions is beneficial. Verify that none of the new versions introduce breaking changes or ESM-only behavior that could affect existing consumers.Consider running integration tests or a quick smoke test against a UniFi controller to confirm compatibility.
unifi.js (10)
27-31: Approve ESM import migration
Convertingrequiretoimportforeventemitter2,ws,axios, andtough-cookieis consistent with"type": "module". All imports appear correct.
113-115: Propagate custom timeout to login requests
Injectingtimeout: this.opts.timeoutinto the_instance.postcall ensures the login respects the configured timeout. Good addition.
1147-1150: Review REST update payload
IneditClientFixedIP, you construct:const payload = { _id: client_id, use_fixedip };Since the HTTP method is PUT, ensure it's the expected shape by the API. Otherwise, it looks consistent.
1703-1710: Confirm admin invitation payload fields
The newfor_sso,permissions, androlemappings should match the UniFi API. Double-check the downstream behavior when grantingAPI_DEVICE_ADOPTorAPI_DEVICE_RESTART.Also applies to: 1747-1751
3154-3156: AddcreateAxiosInstancecustomization hook
Supportingopts.createAxiosInstanceallows consumers to inject a customaxios(e.g., with specialized agents). Ensure your README and example (examples/custom-axios-client.js) clearly describe the function signature and usage.
3164-3164: Override axios status validation for init
UsingvalidateStatus: () => trueensures your UniFiOS detection logic sees all HTTP statuses. Confirm that later code correctly handles non-2xx responses from this call.
3239-3239: Configure timeout for all requests
Addingtimeout: this.opts.timeoutto the generic_requestcall aligns with your login timeout. Approved.
3291-3291: Ensure initialization on first use
The guard in_ensureLoggedIn(if this._instance === undefined) properly triggers_init. Good defensive coding.
3308-3308: Double-check URL rewriting for UniFiOS
The_urlmethod distinguishes UniFiOS vs. classic paths. Confirm that endpoints like/loginand/logoutaren’t inadvertently proxied for UniFiOS.
3316-3316: Consistent default export
Exporting{ Controller }as defaultUnifialigns with your examples and ESM conventions. Approved.
| }, | ||
| "engines": { | ||
| "node": ">=14.18.0 <15.0.0 || >=16.0.0" | ||
| "node": ">=22.0.0" |
There was a problem hiding this comment.
Align Node.js engine with CI matrix
The engines field requires Node ≥ 22.0.0, but the CI workflow still tests on Node.js 20.x. This mismatch will cause installation failures on Node 20. Either update the CI matrix to drop Node 20 or relax the engine requirement.
There was a problem hiding this comment.
Relax the engine requirement please, as Node 20 is still a maintained LTS for a full year.
There was a problem hiding this comment.
@lynoure I'll help you relax the Node.js engine requirement to align with your CI workflow.
Based on the CI matrix that tests Node.js 20.x, 22.x, 23.x, and 24.x, here's the suggested change for package.json:
"engines": {
"node": ">=20.0.0"
}This change will:
- Allow installation on Node.js 20.x (matching your CI workflow)
- Still support all newer LTS and current versions
- Resolve the mismatch between CI testing and package installation requirements
The change should be made on line 53 in package.json, replacing "node": ">=22.0.0" with "node": ">=20.0.0".
|
As a note, this is a massive reduction of node versions, including dropping an LTS Iron a full year before its EOL. Please don't. |
| }, | ||
| "dependencies": { | ||
| "axios": "1.7.9", | ||
| "axios": "1.9.0", |
There was a problem hiding this comment.
1.11.0 ought to also be compatible, as there has been an unfortunate passing of time.
|
First of all, sorry for the way too long delay in reviewing this PR, but it is quite hard right now to keep up with all my side projects like this one here ;-) After having reviewed this PR I have to say it is unfortunately way too stuffed with different types of changes, starting from simple dependency updates until complex ESM module adaptions. And especially the ESM adaptions would have to be discussed separately, I am afraid as node-unifi is currently quite heavily used by certain CommonJS projects which would immediately loose compatibility. So your suggested changes are quite heavily breaking changes which should be separated into a dedicated PR rather than having a common/unified PR for different type of changes. So please try to break down your changes into several smaller pieces and submit a PR for each of them separately. In addition, I would like to ask you to stop distributing an own competitive node-unifi version on npm.com and rather contribute here directly via PR and then ask me to publish these changes. |
|
Hey, thank you for taking the time and looking over the changes, I totally understand that the migration to esm is not something that can just be done. The changes were mostly done to allow better development velocity, but I see that we might want to keep using commonjs for now. Regarding the package, I'm happy to switch back to the official node-unifi package if our requirements allow it. So it doesn't contain critical vulnerabilities and supports a custom client where the user can configure trusted certificates on our side. Thanks for your work ❤️ |
|
Ok, then please do so and break down this large PR in several smaller pieces with PRs for every smaller change. Then we can close here and discuss in the other PRs |
3 participants
Hey, hope you are doing well. We use this library for our open source dashboard project Homarr and one of the features we need, is to customize the axios instance to allow certain certificates. Because of that I've forked the repository and added the required feature. Additionally I've updated all dependencies to the latest version. Because of that I've also migrated to ESM, as this is required by libraries like
xo. The node versions were updated to the currently maintained / developed versions.We've published this version of the package under @homarr/node-unifi with version
2.6.0and use it for now on our dashboard project. If you think we should change anything, let me know 😄Summary by CodeRabbit
New Features
Refactor
Documentation
Chores