Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support cjs and esm both by tshy #17

Merged
merged 3 commits into from
Dec 15, 2024
Merged

feat: support cjs and esm both by tshy #17

merged 3 commits into from
Dec 15, 2024

Conversation

fengmk2
Copy link
Member

@fengmk2 fengmk2 commented Dec 15, 2024

BREAKING CHANGE: drop Node.js < 18.19.0 support

part of eggjs/egg#3644

eggjs/egg#5257

closes #16

Summary by CodeRabbit

  • New Features

    • Introduced a new ESLint configuration for TypeScript and Node.js.
    • Added a new GitHub Actions workflow for package publishing.
    • Implemented a new TypeScript configuration for enhanced type safety.
    • Created a new test suite for validating worker process behavior.
    • Added a new test suite for verifying child process behavior.
  • Bug Fixes

    • Updated Node.js CI workflow to include newer versions and improved configurations.
  • Documentation

    • Enhanced README with updated badges and installation instructions.
    • Updated example documentation to reflect file extension changes.
  • Chores

    • Removed outdated files and workflows to streamline the repository.
    • Updated .gitignore to exclude additional files and directories.

BREAKING CHANGE: drop Node.js < 18.19.0 support

part of eggjs/egg#3644

eggjs/egg#5257
Copy link

coderabbitai bot commented Dec 15, 2024

Warning

Rate limit exceeded

@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 19 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 2b35d84 and 7bf61f0.

📒 Files selected for processing (2)
  • LICENSE (1 hunks)
  • README.md (4 hunks)

Walkthrough

This pull request introduces a comprehensive modernization of the graceful Node.js package, transitioning from a JavaScript-based implementation to a TypeScript-based project. The changes include updating the project structure, introducing TypeScript configuration, modifying GitHub Actions workflows, updating dependencies, and refactoring the core implementation. The modifications aim to improve type safety, module compatibility, and overall project maintainability.

Changes

File/Directory Change Summary
.eslintrc Added ESLint configuration extending TypeScript and Node.js prefix rules
.github/workflows/ Updated Node.js CI, release, and added new package publishing workflow
.gitignore Added new ignore patterns for TypeScript and build artifacts
README.md Updated badges, installation instructions, and code examples
example/express_with_cluster/ Renamed .js files to .cjs and updated import statements
package.json Significant updates to dependencies, scripts, and module configuration
src/index.ts New TypeScript implementation of graceful shutdown mechanism
test/ Migrated tests from JavaScript to TypeScript, added new test cases
tsconfig.json Added TypeScript configuration with strict type checking

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant Graceful as Graceful Module
    participant Server as HTTP Server
    participant Process as Node.js Process

    App->>Graceful: Configure graceful shutdown
    Graceful->>Server: Set up connection handling
    Graceful->>Process: Configure error handling
    
    alt Unexpected Error
        Process->>Graceful: Trigger error
        Graceful->>Server: Initiate graceful shutdown
        Graceful->>Process: Close connections
    end

    alt Timeout Reached
        Graceful->>Process: Force process termination
    end
Loading

Poem

🐰 A Rabbit's Ode to Graceful Code

In TypeScript's embrace, our module now gleams,
Shutting down servers with elegant schemes.
No more abrupt ends, no crashes in sight,
Graceful and smooth, our package takes flight! 🚀

— CodeRabbit's Poetic Deployment 🌈


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

socket-security bot commented Dec 15, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@types/express@5.0.0 None +9 106 kB types
npm/@types/supertest@6.0.2 Transitive: filesystem, network +9 344 kB types
npm/egg-bin@6.13.0 environment Transitive: eval, filesystem, shell, unsafe +196 16.6 MB fengmk2
npm/humanize-ms@2.0.0 None 0 7.25 kB fengmk2
npm/mm@3.4.0 network, shell +7 97.9 kB fengmk2
npm/supertest@7.0.0 network Transitive: environment, eval, filesystem +40 1.92 MB titanism
npm/typescript@5.7.2 None 0 22.7 MB typescript-bot

🚮 Removed packages: npm/egg-bin@5.19.0, npm/humanize-ms@1.2.1, npm/ps-tree@1.2.0, npm/supertest@1.2.0

View full report↗︎

Copy link

pkg-pr-new bot commented Dec 15, 2024

Open in Stackblitz

npm i https://pkg.pr.new/node-modules/graceful@17

commit: 7bf61f0

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🔭 Outside diff range comments (1)
example/failure.cjs (1)

Line range hint 31-34: Verify server array property name

The configuration uses server property but README shows servers. Ensure consistency across examples.

 graceful({
-  server: [app, app1],
+  servers: [app, app1],
   killTimeout: '10s',
 });
🧰 Tools
🪛 eslint

[error] 3-3: Unexpected var, use let or const instead.

(no-var)


[error] 3-3: 'http' module is restricted from being used.

(no-restricted-modules)


[error] 3-3: Require statement not part of import statement.

(@typescript-eslint/no-var-requires)


[error] 4-4: Unexpected var, use let or const instead.

(no-var)


[error] 4-4: Require statement not part of import statement.

(@typescript-eslint/no-var-requires)


[error] 5-5: Unexpected var, use let or const instead.

(no-var)


[error] 5-5: Require statement not part of import statement.

(@typescript-eslint/no-var-requires)

🧹 Nitpick comments (16)
.github/workflows/pkg.pr.new.yml (1)

2-2: Consider limiting workflow triggers

Running on both push and pull_request events could lead to duplicate runs when PRs are updated. Consider limiting to either pull_request or specific push events.

-on: [push, pull_request]
+on:
+  pull_request:
+  push:
+    branches: [ master ]
src/index.ts (3)

75-75: Use direct property assignment instead of Reflect.set

At line 75, you are using Reflect.set(req, 'shouldKeepAlive', false);. If req.shouldKeepAlive is writable, you can assign it directly for clarity:

req.shouldKeepAlive = false;

85-93: Remove 'async' keyword from setTimeout callback

The setTimeout callback at line 85 is declared as async, but setTimeout does not handle the returned promise. This may lead to unhandled promise rejections. Consider removing the async keyword or handling promises within the function appropriately.

Proposed change:

- const killTimer = setTimeout(async () => {
+ const killTimer = setTimeout(() => {

115-120: Avoid using 'any' type in catch blocks

In the catch blocks starting at lines 115 and 130, err is typed as any. Using any bypasses TypeScript's type checking. Consider typing err as unknown to maintain type safety.

Proposed changes:

- } catch (err: any) {
+ } catch (err) {

If you need to access properties on err, you can perform type assertions or checks.

Also applies to: 130-135

test/fixtures/app.cjs (1)

1-1: Adjust ESLint configuration for CommonJS files

The ESLint rule @typescript-eslint/no-var-requires is disabled in this CommonJS file. Since .cjs files use CommonJS syntax, consider adjusting your ESLint configuration to apply TypeScript rules only to .ts files, eliminating the need for disabling this rule.

test/fixtures/ignore.cjs (1)

1-1: Adjust ESLint configuration for CommonJS files

Similar to app.cjs, you have disabled @typescript-eslint/no-var-requires. Consider configuring ESLint to differentiate between TypeScript and CommonJS files to avoid globally disabling rules.

test/ignore.test.ts (1)

23-30: Add proper error typing and documentation

The alive function should have proper error typing and documentation.

Consider this improvement:

+/**
+ * Check if a process is alive
+ * @param pid - Process ID to check
+ * @returns boolean indicating if process is alive
+ */
-function alive(pid: number) {
+function alive(pid: number): boolean {
   try {
     process.kill(pid, 0);
     return true;
-  } catch (err) {
+  } catch (err: unknown) {
     return false;
   }
 }
test/worker.test.ts (1)

1-1: Consider additional improvements for dual CJS/ESM support

While the PR successfully adds support for both CJS and ESM modules, consider these additional improvements:

  1. Add type declarations (.d.ts files) for both CJS and ESM usage
  2. Document the dual module support in the README
  3. Add examples demonstrating both import styles
  4. Consider adding integration tests that verify both module systems work as expected

Would you like help implementing any of these suggestions?

package.json (1)

53-72: Well-structured dual module format support

The exports configuration correctly handles both ESM and CommonJS formats with proper type definitions. The setup follows Node.js best practices for dual-format packages.

Consider adding a comment in the README about the dual module support and minimum Node.js version requirement to help users understand the breaking changes.

test/graceful.test.ts (3)

18-21: Consider improving TypeScript type safety

Multiple @ts-ignore comments suggest potential type safety improvements. Consider creating proper mock objects instead of relying on undefined variables.

Example refactor for the first error case:

-process.nextTick(function() {
-  // @ts-ignore
-  ff.foo();
-});
+process.nextTick(function() {
+  const error = new Error('Simulated async error');
+  process.emit('uncaughtException', error);
+});

Also applies to: 25-28, 29-32, 36-47


13-51: Consider refactoring the request handler

The request handler could be simplified using a map of routes to handlers, improving maintainability.

const errorHandlers = {
  '/sync_error': () => {
    throw new Error('sync_error');
  },
  '/async_error': () => {
    process.nextTick(() => {
      throw new Error('async_error');
    });
  },
  // ... other handlers
};

function normalHandler(req: express.Request, res: express.Response) {
  const handler = errorHandlers[req.url];
  if (handler) {
    handler();
    return;
  }
  res.end(req.url);
}

53-56: Add proper Express types

The error handler should use proper Express types for better type safety.

-function errorHandler(err: any, _req: any, res: any) {
+function errorHandler(
+  err: Error,
+  _req: express.Request,
+  res: express.Response,
+  _next: express.NextFunction
+) {
   res.statusCode = 500;
   res.end(err.message);
 }
README.md (1)

93-94: Update copyright year format

Consider using en dash for year ranges for better readability.

-Copyright (c) 2013 - 2014 fengmk2
-Copyright (c) 2015 - present node-modules and other contributors
+Copyright (c) 2013–2014 fengmk2
+Copyright (c) 2015–present node-modules and other contributors
🧰 Tools
🪛 LanguageTool

[typographical] ~93-~93: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...e (The MIT License) Copyright (c) 2013 - 2014 fengmk2 Copyright (c) 2015 - presen...

(DASH_RULE)

example/failure.cjs (1)

5-5: Modernize variable declarations

Update variable declarations to use modern JavaScript syntax.

-var { graceful } = require('../');
+const { graceful } = require('../');
🧰 Tools
🪛 eslint

[error] 5-5: Unexpected var, use let or const instead.

(no-var)


[error] 5-5: Require statement not part of import statement.

(@typescript-eslint/no-var-requires)

example/express_with_cluster/README.md (2)

Line range hint 27-45: Update example output timestamps

The example output shows dates from 2013. Consider updating to current dates for clarity.

Update the example output with current dates and consider removing specific timestamps to make the example more timeless.


10-10: Remove $ prefix from command examples

For better copy-paste experience, remove the $ prefix from command examples.

-$ node example/express_with_cluster/dispatch.cjs
+node example/express_with_cluster/dispatch.cjs
🧰 Tools
🪛 Markdownlint (0.37.0)

10-10: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fcc80da and e3ef17b.

📒 Files selected for processing (27)
  • .eslintrc (1 hunks)
  • .github/workflows/codeql-analysis.yml (0 hunks)
  • .github/workflows/nodejs.yml (1 hunks)
  • .github/workflows/pkg.pr.new.yml (1 hunks)
  • .github/workflows/release.yml (1 hunks)
  • .gitignore (1 hunks)
  • README.md (5 hunks)
  • example/express_with_cluster/README.md (2 hunks)
  • example/express_with_cluster/dispatch.cjs (1 hunks)
  • example/express_with_cluster/worker.cjs (1 hunks)
  • example/failure.cjs (1 hunks)
  • index.js (0 hunks)
  • package.json (1 hunks)
  • src/index.ts (1 hunks)
  • test/fixtures/app.cjs (1 hunks)
  • test/fixtures/app.js (0 hunks)
  • test/fixtures/foo.js (1 hunks)
  • test/fixtures/ignore.cjs (1 hunks)
  • test/fixtures/ignore.js (0 hunks)
  • test/fixtures/worker.cjs (0 hunks)
  • test/graceful.test.js (0 hunks)
  • test/graceful.test.ts (1 hunks)
  • test/ignore.test.js (0 hunks)
  • test/ignore.test.ts (1 hunks)
  • test/worker.test.js (0 hunks)
  • test/worker.test.ts (1 hunks)
  • tsconfig.json (1 hunks)
💤 Files with no reviewable changes (8)
  • test/fixtures/worker.cjs
  • test/fixtures/app.js
  • test/fixtures/ignore.js
  • test/worker.test.js
  • test/ignore.test.js
  • .github/workflows/codeql-analysis.yml
  • test/graceful.test.js
  • index.js
✅ Files skipped from review due to trivial changes (3)
  • .eslintrc
  • .gitignore
  • test/fixtures/foo.js
🧰 Additional context used
🪛 eslint
example/express_with_cluster/dispatch.cjs

[error] 6-7: Missing trailing comma.

(comma-dangle)

example/express_with_cluster/worker.cjs

[error] 4-4: Unexpected var, use let or const instead.

(no-var)


[error] 4-4: Require statement not part of import statement.

(@typescript-eslint/no-var-requires)

example/failure.cjs

[error] 5-5: Unexpected var, use let or const instead.

(no-var)


[error] 5-5: Require statement not part of import statement.

(@typescript-eslint/no-var-requires)

🪛 LanguageTool
README.md

[typographical] ~93-~93: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...e (The MIT License) Copyright (c) 2013 - 2014 fengmk2 Copyright (c) 2015 - presen...

(DASH_RULE)

🪛 Markdownlint (0.37.0)
example/express_with_cluster/README.md

10-10: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

🔇 Additional comments (9)
.github/workflows/release.yml (1)

10-10: Verify dual module format support in release workflow

Since this PR adds support for both CJS and ESM formats, ensure that the referenced workflow node-modules/github-actions/.github/workflows/node-release.yml@master properly handles publishing of dual-format packages.

Consider documenting the release process for dual module formats in the repository's README.md to help maintainers understand how the packages are published.

✅ Verification successful

Since we cannot directly access the external workflow file, let's verify if the current repository has dual module format configuration and documentation.


Dual module format configuration is properly set up

The package.json confirms proper dual module format configuration with:

  • ESM support via "type": "module"
  • CJS output at "main": "./dist/commonjs/index.js"
  • ESM output at "module": "./dist/esm/index.js"
  • Conditional exports properly configured for both formats

Since the repository has the correct dual format configuration and the workflow is from the official node-modules organization (same as the repository owner), we can reasonably expect the release workflow to handle publishing correctly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the referenced workflow supports dual module format publishing
gh api repos/node-modules/github-actions/contents/.github/workflows/node-release.yml | jq -r '.content' | base64 -d | grep -A 10 "npm.*publish"

Length of output: 240


Script:

#!/bin/bash
# Check package.json for dual module format configuration
rg -A 5 '"type":|"main":|"module":|"exports":' package.json

# Check for documentation about release process
rg -i "release|publish" README.md

Length of output: 639

.github/workflows/nodejs.yml (2)

15-15: LGTM: Node.js versions align with requirements

The specified Node.js versions (18.19.0, 18, 20, 22) correctly align with the PR objective of dropping support for Node.js versions < 18.19.0.


Line range hint 16-18: Verify test coverage for both CJS and ESM

With the addition of dual module format support, ensure that the test coverage reporting includes both CJS and ESM usage scenarios.

.github/workflows/pkg.pr.new.yml (2)

20-21: Verify build script handles dual module formats

The prepublishOnly script should handle building both CJS and ESM formats.

✅ Verification successful

Build script correctly handles dual module formats

The package uses tshy and tshy-after for building both CJS and ESM formats, as evidenced by:

  • Package configuration shows proper dual format setup with "type": "module" and exports field defining both ESM and CJS paths
  • The prepublishOnly script uses tshy which is a specialized tool for building TypeScript hybrid packages
  • Output paths are correctly configured in package.json:
    • ESM: ./dist/esm/index.js
    • CJS: ./dist/commonjs/index.js
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check package.json for dual format build configuration
echo "Checking build scripts:"
fd "package.json" --exec cat {} | jq '.scripts | select(.prepublishOnly != null)'

echo -e "\nChecking package.json for dual format configuration:"
fd "package.json" --exec cat {} | jq 'select(.type != null or .exports != null or .module != null)'

Length of output: 2594


23-23: Verify pkg-pr-new compatibility with dual formats

Ensure that pkg-pr-new correctly handles publishing packages with both CJS and ESM formats.

tsconfig.json (1)

1-10: TypeScript configuration is appropriate

The tsconfig.json is well-configured for modern Node.js environments with ESM support. Using "module": "NodeNext" and "moduleResolution": "NodeNext" ensures compatibility with both CommonJS and ESM modules.

example/express_with_cluster/dispatch.cjs (1)

6-6: LGTM: Consistent file extension update

The change from worker.js to worker.cjs aligns with the PR's objective of explicit module format support.

🧰 Tools
🪛 eslint

[error] 6-7: Missing trailing comma.

(comma-dangle)

package.json (1)

24-27: Verify dependency compatibility

The updated dependencies need verification for compatibility with both CJS and ESM.

✅ Verification successful

Dependencies are compatible with dual CJS/ESM mode

The project is properly configured for dual CJS/ESM support:

  • Package.json defines both CJS and ESM exports
  • TypeScript is configured with "moduleResolution": "NodeNext"
  • Dependencies are imported using ESM syntax in the codebase
  • Both dependencies are used in production code and are essential
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if dependencies support both CJS and ESM
npx attw --pack .

Length of output: 113


Script:

#!/bin/bash
# Check package.json for type field and main/module fields
cat package.json

# Check if dependencies have their own package.json files
fd -t f "package.json" -x cat {} \; --exec-batch

Length of output: 2146


Script:

#!/bin/bash
# Check if the dependencies are dual-mode compatible by examining their package.json files in node_modules
rg -A 10 '"@fengmk2/ps-tree":|"humanize-ms":' node_modules/*/package.json

# Check for any import/require statements in the codebase
rg -l "require\(.*(@fengmk2/ps-tree|humanize-ms)" .
rg -l "import.*from.*(@fengmk2/ps-tree|humanize-ms)" .

Length of output: 762


Script:

#!/bin/bash
# Check how these dependencies are being used in the codebase
rg -B2 -A2 "(@fengmk2/ps-tree|humanize-ms)" ./src/index.ts ./test/worker.test.ts

# Check the project's build configuration for dual-mode support
cat tsconfig.json

Length of output: 855

example/express_with_cluster/README.md (1)

3-5: Verify dual module system support

Let's verify that both CJS and ESM imports are properly supported across the codebase.

✅ Verification successful

CommonJS format is correctly used in example code

The example code in express_with_cluster directory consistently uses CommonJS format (.cjs extension) with require() statements throughout all files:

  • dispatch.cjs uses require('cluster') and require('path')
  • worker.cjs uses require('../../') and require('./app')
  • app.cjs uses require('http') and require('express')

The package.json exports configuration shows proper dual module system support at the package level, with both ESM and CommonJS builds available. The example code's use of .cjs extension is intentional and correct for demonstrating CommonJS usage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify module system support across the codebase

# Check for CJS require statements
echo "Checking CommonJS require statements:"
rg "require\(['\"]graceful['\"]" -g "!{*.md,package.json}"

# Check for ESM import statements
echo "Checking ESM import statements:"
rg "import.*from ['\"]graceful['\"]" -g "!{*.md,package.json}"

# Check for package.json exports field
echo "Checking package.json exports configuration:"
cat package.json | jq '.exports'

Length of output: 692


Script:

#!/bin/bash
# Let's check the actual files in the example directory to verify module system support

# Check files in express_with_cluster example
echo "Files in express_with_cluster example:"
ls -la example/express_with_cluster/

# Check content of the mentioned files
echo -e "\nContent of dispatch.cjs:"
cat example/express_with_cluster/dispatch.cjs

echo -e "\nContent of worker.cjs:"
cat example/express_with_cluster/worker.cjs

echo -e "\nContent of app.cjs:"
cat example/express_with_cluster/app.cjs

Length of output: 4129

src/index.ts Show resolved Hide resolved
example/express_with_cluster/worker.cjs Show resolved Hide resolved
test/ignore.test.ts Show resolved Hide resolved
Comment on lines +38 to +47
function alive(pid: number) {
try {
process.kill(pid, 0);
console.warn('%s alive', pid);
return true;
} catch (err) {
console.error('kill %s error: %s', pid, err);
return false;
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove console logging from test utility function

The alive function contains console logging which is not ideal for test code. Consider using proper test assertions instead.

Apply this improvement:

 function alive(pid: number) {
   try {
     process.kill(pid, 0);
-    console.warn('%s alive', pid);
     return true;
   } catch (err) {
-    console.error('kill %s error: %s', pid, err);
     return false;
   }
 }
📝 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.

Suggested change
function alive(pid: number) {
try {
process.kill(pid, 0);
console.warn('%s alive', pid);
return true;
} catch (err) {
console.error('kill %s error: %s', pid, err);
return false;
}
}
function alive(pid: number) {
try {
process.kill(pid, 0);
return true;
} catch (err) {
return false;
}
}

test/worker.test.ts Show resolved Hide resolved
Comment on lines +37 to +38
const express = require('express');
const { graceful } = require('graceful');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add ESM import example alongside CJS

The example only shows CommonJS require syntax, but the PR adds ESM support. Should include both import styles for completeness.

Add ESM example:

+// CommonJS
 const express = require('express');
 const { graceful } = require('graceful');

+// ESM
+import express from 'express';
+import { graceful } from 'graceful';

Also applies to: 40-40, 63-63

README.md Show resolved Hide resolved
Copy link

codecov bot commented Dec 15, 2024

Codecov Report

Attention: Patch coverage is 33.95062% with 107 lines in your changes missing coverage. Please review.

Project coverage is 33.95%. Comparing base (c03a363) to head (7bf61f0).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/index.ts 33.95% 107 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
+ Coverage   25.37%   33.95%   +8.57%     
==========================================
  Files           1        1              
  Lines          67      162      +95     
  Branches       16        4      -12     
==========================================
+ Hits           17       55      +38     
- Misses         50      107      +57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
README.md (4)

26-26: Document TypeScript and dual module support in installation section

Since the package now supports TypeScript and dual module formats (CJS/ESM), consider adding this information to help users:

 npm install graceful
+
+// TypeScript types are included
+// Supports both CommonJS and ES Modules

33-33: Fix grammatical error in warning message

Change "don't use it on production env" to "don't use it in production environment"

🧰 Tools
🪛 LanguageTool

[uncategorized] ~33-~33: The preposition “in” seems more likely in this position than the preposition “on”.
Context: ...ow code just for dev demo, don't use it on production env: ```js const express = ...

(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_ON_IN)


79-83: Enhance ESM and TypeScript documentation section

The section could be more comprehensive by including:

  • TypeScript configuration examples
  • Module resolution settings
  • Type definitions usage

Add expanded documentation:

 ### ESM and TypeScript
 
 ```ts
 import { graceful } from 'graceful';

+#### TypeScript Configuration
+```json
+{

  • "compilerOptions": {
  • "moduleResolution": "node16",
  • // ... other options
  • }
    +}
    +```

+#### Type Definitions
+```ts
+import type { GracefulOptions } from 'graceful';
+
+const options: GracefulOptions = {

  • servers: [server],
  • killTimeout: '30s',
    +};
    +```

---

`95-96`: **Use en dashes for year ranges**

Replace hyphens with en dashes in copyright year ranges:
```diff
-Copyright (c) 2013 - 2014 fengmk2
-Copyright (c) 2015 - present node-modules and other contributors
+Copyright (c) 2013–2014 fengmk2
+Copyright (c) 2015–present node-modules and other contributors
🧰 Tools
🪛 LanguageTool

[typographical] ~95-~95: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...e (The MIT License) Copyright (c) 2013 - 2014 fengmk2 Copyright (c) 2015 - presen...

(DASH_RULE)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e3ef17b and 2b35d84.

📒 Files selected for processing (1)
  • README.md (4 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md

[uncategorized] ~33-~33: The preposition “in” seems more likely in this position than the preposition “on”.
Context: ...ow code just for dev demo, don't use it on production env: ```js const express = ...

(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_ON_IN)


[typographical] ~95-~95: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...e (The MIT License) Copyright (c) 2013 - 2014 fengmk2 Copyright (c) 2015 - presen...

(DASH_RULE)

🔇 Additional comments (2)
README.md (2)

6-6: Enhance Node.js version requirement visibility

While the Node.js version badge is helpful, it should be more explicit about the breaking change in version requirements.


36-37: Add ESM import example alongside CJS

The example should demonstrate both import styles since the package now supports dual modules.

@fengmk2 fengmk2 merged commit 7192a67 into master Dec 15, 2024
16 checks passed
@fengmk2 fengmk2 deleted the egg-v4 branch December 15, 2024 08:42
fengmk2 pushed a commit that referenced this pull request Dec 15, 2024
[skip ci]

## [2.0.0](v1.1.0...v2.0.0) (2024-12-15)

### ⚠ BREAKING CHANGES

* drop Node.js < 18.19.0 support

part of eggjs/egg#3644

eggjs/egg#5257

closes #16

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
	- Introduced a new ESLint configuration for TypeScript and Node.js.
	- Added a new GitHub Actions workflow for package publishing.
	- Implemented a new TypeScript configuration for enhanced type safety.
	- Created a new test suite for validating worker process behavior.
	- Added a new test suite for verifying child process behavior.

- **Bug Fixes**
- Updated Node.js CI workflow to include newer versions and improved
configurations.

- **Documentation**
	- Enhanced README with updated badges and installation instructions.
	- Updated example documentation to reflect file extension changes.

- **Chores**
	- Removed outdated files and workflows to streamline the repository.
	- Updated `.gitignore` to exclude additional files and directories.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

### Features

* support cjs and esm both by tshy ([#17](#17)) ([7192a67](7192a67))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ps-tree error on node.js 20
1 participant