feat: refactor with TypeScript #5
Conversation
BREAKING CHANGE: drop Node.js < 14 support eggjs/egg#5257
WalkthroughThe changes in this pull request involve the removal of several configuration and source files, along with the introduction of new GitHub Actions workflows and TypeScript configurations. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CI
participant Node
participant Repo
User->>Repo: Push changes
Repo->>CI: Trigger CI workflow
CI->>Node: Setup Node.js
CI->>Node: Install dependencies
CI->>Node: Run tests
CI->>Repo: Publish package
CI->>Repo: Release new version
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/autod@3.1.2, npm/benchmark@1.0.0, npm/istanbul@0.4.5, npm/mocha@11.0.2, npm/should@13.2.3 |
commit: |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
src/index.ts (2)
6-6: Consider adding more specific type definitionsThe parameter type
number | stringcould be more specific to better document the expected string format (e.g., '1s', '1m', '1h').-export function ms(t: number | string) { +type TimeString = `${number}${'ms' | 's' | 'm' | 'h' | 'd' | 'w' | 'y'}`; +export function ms(t: number | TimeString) {
3-5: Enhance function documentationThe current JSDoc comment could be more descriptive with parameter types, return type, and examples.
/** - * transform humanize time to ms + * Transforms a human-readable time string or number to milliseconds + * @param t - Time in milliseconds or a string like '1s', '1m', '1h' + * @returns The time in milliseconds + * @throws {TypeError} When the input format is invalid + * @example + * ms('2h') // returns 7200000 + * ms(2000) // returns 2000 */.github/workflows/nodejs-14.yml (1)
1-8: Consider adding test coverage and artifact uploadThe workflow could benefit from additional steps to track test coverage and preserve test results.
name: Node.js 14 CI on: push: branches: [ master ] pull_request: branches: [ master ] + paths: + - '**.ts' + - 'package*.json' + - '.github/workflows/nodejs-14.yml'Consider adding these steps after running tests:
- name: Upload test coverage uses: actions/upload-artifact@v4 with: name: coverage-report path: coverage/test/index.test.ts (2)
7-8: Enhance type safety in test assertionsConsider using TypeScript's type assertions to make the tests more type-safe.
- assert.equal(ms(1000), 1000); + assert.equal(ms(1000), 1000 as number);
19-23: Consider testing error handling more thoroughlyThe invalid string test case could be expanded to cover more edge cases.
Consider adding more test cases:
it('should handle various invalid inputs', () => { assert.equal(ms(''), undefined); assert.equal(ms('invalid'), undefined); assert.equal(ms('1x'), undefined); assert.equal(ms('ms'), undefined); });README.md (1)
28-33: Consider documenting both import stylesWhile the TypeScript/ESM import is now the primary method, consider documenting the CommonJS require syntax for users who haven't migrated to ESM yet:
// ESM / TypeScript import { ms } from 'humanize-ms'; // CommonJS const { ms } = require('humanize-ms');package.json (1)
47-66: LGTM: Proper dual package hazard handlingThe export configuration correctly handles both ESM and CommonJS formats. Consider adding tests to verify both import methods work as expected:
// test/esm.test.ts import { ms } from 'humanize-ms'; // test/commonjs.test.ts const { ms } = require('humanize-ms');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.editorconfig(0 hunks).eslintrc(1 hunks).github/workflows/nodejs-14.yml(1 hunks).github/workflows/nodejs.yml(1 hunks).github/workflows/pkg.pr.new.yml(1 hunks).github/workflows/release.yml(1 hunks).gitignore(1 hunks).jshintignore(0 hunks).jshintrc(0 hunks).travis.yml(0 hunks)LICENSE(1 hunks)Makefile(0 hunks)README.md(1 hunks)index.js(0 hunks)package.json(1 hunks)src/index.ts(1 hunks)test/benchmark/ms.cjs(1 hunks)test/benchmark/ms.js(0 hunks)test/index.test.js(0 hunks)test/index.test.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (8)
- .travis.yml
- .jshintignore
- index.js
- test/benchmark/ms.js
- .editorconfig
- test/index.test.js
- Makefile
- .jshintrc
✅ Files skipped from review due to trivial changes (5)
- LICENSE
- .eslintrc
- .gitignore
- .github/workflows/nodejs.yml
- tsconfig.json
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 446 characters long)
Context: ...s [![NPM version][npm-image]][npm-url] 
[![Test coverage][codecov-image]][codecov-url] [![Known Vulnerabilities][snyk-image]][sny...
(EN_EXCESSIVE_EXCLAMATION)
🪛 eslint
test/benchmark/ms.cjs
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 4-4: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🔇 Additional comments (9)
.github/workflows/release.yml (1)
3-13: Verify branch protection and secret configuration
The workflow looks good but requires verification of:
- Branch protection rules for
masterto ensure proper release control - Required secrets (
NPM_TOKEN,GIT_TOKEN) are properly configured
.github/workflows/pkg.pr.new.yml (1)
12-16: LGTM: Node.js version aligns with PR objectives
The use of Node.js 20 is appropriate given the PR's objective to drop support for Node.js versions < 14.
test/benchmark/ms.cjs (1)
38-40: LGTM: Performance improvements verified
The benchmark results show that the TypeScript implementation maintains or improves performance compared to the original:
- Similar performance for string parsing (
ms()vsorigin ms()) - Significantly better performance for number handling (
ms(1000))
README.md (3)
1-16: LGTM: CI/CD modernization
The badge updates appropriately reflect the migration to GitHub Actions and modern tooling.
🧰 Tools
🪛 LanguageTool
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 446 characters long)
Context: ...s [![NPM version][npm-image]][npm-url] [![Test coverage][codecov-image]][codecov-url] [![Known Vulnerabilities][snyk-image]][sny...
(EN_EXCESSIVE_EXCLAMATION)
22-24: LGTM: Improved installation instructions
The simplified installation command enhances readability.
37-43: LGTM: Enhanced project documentation
The addition of the contributors section and updated license reference improve the project documentation.
package.json (3)
40-46: LGTM: Comprehensive development workflow
The new scripts properly support TypeScript development workflow with linting, testing, and CI integration.
67-73: LGTM: Proper TypeScript package configuration
The package files configuration correctly includes all necessary files and properly sets up type definitions.
22-39: Verify ms package update implications
The update of ms from ^2.0.0 to ^2.1.3 should be verified for:
- Breaking changes in the API
- Security implications
✅ Verification successful
Let me check the usage of the ms package in the codebase to understand the impact of the update.
Update to ms@^2.1.3 is safe to proceed
The update from ^2.0.0 to ^2.1.3 is safe because:
- No security vulnerabilities affect versions >= 2.0.0 (latest advisory shows vulnerabilities only for < 2.0.0)
- The codebase usage aligns with ms's stable API:
- Only using basic time string parsing (
ms('1s')) and number passthrough (ms(1000)) - All test cases pass with expected behavior
- Only using basic time string parsing (
- It's a minor version bump (2.0.x → 2.1.x) following semver, indicating no breaking changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check ms package changelog and security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "ms") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 771
Script:
#!/bin/bash
# Search for ms package usage
rg -l "require\(['\"](ms)['\"]" || rg -l "from ['\"](ms)['\"]"
# Check package-lock.json for current version
if [ -f "package-lock.json" ]; then
cat package-lock.json | jq '.packages["node_modules/ms"]'
fi
# Search for direct ms() function calls or time string parsing
rg "ms\(" -A 2
Length of output: 1632
[skip ci] ## 1.0.0 (2024-12-12) ### ⚠ BREAKING CHANGES * drop Node.js < 14 support eggjs/egg#5257 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new function `ms` for converting human-readable time formats into milliseconds. - Added multiple GitHub Actions workflows for CI, testing, and publishing automation. - Updated TypeScript configuration for improved type safety. - **Bug Fixes** - Removed outdated configuration files that could lead to linting errors or CI issues. - **Documentation** - Updated `README.md` with new CI/CD references, installation instructions, and contributor acknowledgments. - Added a license section with the MIT License. - **Chores** - Updated `package.json` with new dependencies and scripts for linting and testing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * **index.js:** warn when result is undefined ([b89dae5](b89dae5)) * refactor with TypeScript ([#5](#5)) ([14964af](14964af)) ### Bug Fixes * package.json to reduce vulnerabilities ([#3](#3)) ([3b6f1ba](3b6f1ba))
[skip ci] * drop Node.js < 14 support eggjs/egg#5257 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Introduced a new function `ms` for converting human-readable time formats into milliseconds. - Added multiple GitHub Actions workflows for CI, testing, and publishing automation. - Updated TypeScript configuration for improved type safety. - **Bug Fixes** - Removed outdated configuration files that could lead to linting errors or CI issues. - **Documentation** - Updated `README.md` with new CI/CD references, installation instructions, and contributor acknowledgments. - Added a license section with the MIT License. - **Chores** - Updated `package.json` with new dependencies and scripts for linting and testing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> * **index.js:** warn when result is undefined ([b89dae5](b89dae5)) * refactor with TypeScript ([#5](#5)) ([14964af](14964af)) * package.json to reduce vulnerabilities ([#3](#3)) ([3b6f1ba](3b6f1ba))
[skip ci] * drop Node.js < 14 support eggjs/egg#5257 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Introduced a new function `ms` for converting human-readable time formats into milliseconds. - Added multiple GitHub Actions workflows for CI, testing, and publishing automation. - Updated TypeScript configuration for improved type safety. - **Bug Fixes** - Removed outdated configuration files that could lead to linting errors or CI issues. - **Documentation** - Updated `README.md` with new CI/CD references, installation instructions, and contributor acknowledgments. - Added a license section with the MIT License. - **Chores** - Updated `package.json` with new dependencies and scripts for linting and testing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> * **index.js:** warn when result is undefined ([b89dae5](b89dae5)) * refactor with TypeScript ([#5](#5)) ([14964af](14964af)) * package.json to reduce vulnerabilities ([#3](#3)) ([3b6f1ba](3b6f1ba))
[skip ci] ## 1.0.0 (2024-12-12) ### ⚠ BREAKING CHANGES * drop Node.js < 14 support eggjs/egg#5257 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new function `ms` for converting human-readable time formats into milliseconds. - Added multiple GitHub Actions workflows for CI, testing, and publishing automation. - Updated TypeScript configuration for improved type safety. - **Bug Fixes** - Removed outdated configuration files that could lead to linting errors or CI issues. - **Documentation** - Updated `README.md` with new CI/CD references, installation instructions, and contributor acknowledgments. - Added a license section with the MIT License. - **Chores** - Updated `package.json` with new dependencies and scripts for linting and testing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * **index.js:** warn when result is undefined ([b89dae5](b89dae5)) * refactor with TypeScript ([#5](#5)) ([14964af](14964af)) ### Bug Fixes * package.json to reduce vulnerabilities ([#3](#3)) ([3b6f1ba](3b6f1ba))
BREAKING CHANGE: drop Node.js < 14 support
eggjs/egg#5257
Summary by CodeRabbit
New Features
msfor converting human-readable time formats into milliseconds.Bug Fixes
Documentation
README.mdwith new CI/CD references, installation instructions, and contributor acknowledgments.Chores
package.jsonwith new dependencies and scripts for linting and testing.