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 #14

Merged
merged 6 commits into from
Dec 18, 2024
Merged

feat: support cjs and esm both by tshy #14

merged 6 commits into from
Dec 18, 2024

Conversation

fengmk2
Copy link
Member

@fengmk2 fengmk2 commented Dec 17, 2024

BREAKING CHANGE: drop Node.js < 18.19.0 support

part of eggjs/egg#3644

eggjs/egg#5257

Summary by CodeRabbit

  • New Features

    • Introduced new configuration files for managing watcher settings in different environments (default, local, unittest).
    • Added a new Boot class to manage application lifecycle and watcher initialization.
    • Implemented Watcher class for monitoring file changes with event handling.
    • Added DevelopmentEventSource and DefaultEventSource classes for specific event source management.
  • Bug Fixes

    • Enhanced path handling in various modules to ensure correct file watching functionality.
  • Documentation

    • Updated README.md with project name change and improved structure.
  • Tests

    • Introduced new unit tests for watcher functionality and refactored existing test files to improve clarity and structure.
  • Chores

    • Removed deprecated configuration files and streamlined project structure.
    • Updated TypeScript configuration for stricter type-checking.

BREAKING CHANGE: drop Node.js < 18.19.0 support

part of eggjs/egg#3644

eggjs/egg#5257
Copy link

coderabbitai bot commented Dec 17, 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 13 minutes and 5 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 47472ef and 200cf15.

📒 Files selected for processing (5)
  • .github/workflows/nodejs.yml (1 hunks)
  • README.md (5 hunks)
  • src/lib/event-sources/development.ts (1 hunks)
  • test/development.test.ts (1 hunks)
  • test/development_cluster.test.ts (4 hunks)

Walkthrough

This pull request represents a comprehensive refactoring of the @eggjs/watcher package, transitioning from a JavaScript-based implementation to a TypeScript-based project. The changes involve migrating the codebase to TypeScript, updating the project structure, modernizing the build and testing processes, and enhancing type definitions. Key modifications include restructuring the source code, introducing new workflows for continuous integration and deployment, and updating configuration files to support the new project structure.

Changes

File/Directory Change Summary
.github/workflows/ Added new CI/CD workflows for Node.js testing, package publishing, and release management
package.json Updated package name, added TypeScript configuration, updated dependencies, and modified build scripts
src/ Migrated existing JavaScript files to TypeScript, introducing type definitions and enhanced module structure
config/ Replaced configuration files with TypeScript equivalents
test/ Updated test files to TypeScript, modified utility functions and test configurations
Removed files Deleted legacy configuration files like .travis.yml, .autod.conf.js, and old JavaScript implementation files

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant Repo as GitHub Repository
    participant CI as GitHub Actions
    participant NPM as NPM Registry

    Dev->>Repo: Push changes
    Repo->>CI: Trigger workflows
    CI->>CI: Run tests
    CI->>CI: Build package
    alt Tests Pass
        CI->>NPM: Publish package
        NPM-->>Dev: Package available
    else Tests Fail
        CI->>Dev: Notify failure
    end
Loading

Poem

🐰 A Watcher's Tale of Transformation

From JavaScript's embrace, we leap to TypeScript's grace,
Workflows dancing, tests prancing with newfound might,
Configurations refined, dependencies aligned,
A rabbit's refactor, shining ever so bright! 🌟


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

pkg-pr-new bot commented Dec 18, 2024

Open in Stackblitz

npm i https://pkg.pr.new/eggjs/egg-watcher/@eggjs/watcher@14

commit: 200cf15

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: 9

🔭 Outside diff range comments (1)
README.md (1)

Line range hint 67-94: Document ESM and CJS usage

Since this PR adds support for both ESM and CJS, the documentation should include examples for both module systems.

Add a section explaining both import styles:

+## Module Systems Support
+
+This package supports both ESM and CommonJS:
+
+```ts
+// ESM
+import { Base } from 'sdk-base';
+
+// CommonJS
+const { Base } = require('sdk-base');
+```
🧰 Tools
🪛 LanguageTool

[uncategorized] ~59-~59: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...cal(env is local). Once files on disk is modified it will emit a change event ...

(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)


[uncategorized] ~59-~59: A comma might be missing here.
Context: ...(env is local). Once files on disk is modified it will emit a change event immediate...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)


[uncategorized] ~64-~64: A comma might be missing here.
Context: ...om, eggPlugin.name: watcherCustom`). Firstly define our custom event source like thi...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)

🧹 Nitpick comments (23)
src/lib/watcher.ts (4)

1-3: Consider using named imports for third-party libraries for clarity.
While importing from 'node:util', 'sdk-base', and 'camelcase' is perfectly valid, some teams prefer explicitly naming each import. For example, explicitly naming "debug as debugLog" can help highlight the function’s usage.


47-62: Ensure the watcher’s usage of events matches the Node.js Emit API.
Emitting events for each path (line 61) may flood listeners if done frequently. If performance becomes a concern, consider batching or debouncing path events.


64-87: Consider completing the “unwatch” functionality.
Currently, the unwatch method is commented out (lines 64-87). Without a corresponding unwatch mechanism, watchers could leak resources or file handles if watchers aren’t removed when no longer needed.


103-116: Differentiate fuzzy-change events from regular change events.
Here, fuzzy-change triggers a check reversed from #onChange. This is clear from code comments. However, ensure these semantics are well documented so that future maintainers understand when each event is fired.

src/lib/event-sources/base.ts (1)

1-6: Provide documentation for abstract methods.
Adding TSDoc comments to “watch” and “unwatch” clarifies the expected behavior and possible error conditions for implementers. This is especially helpful given they’re abstract.

test/utils.ts (1)

8-10: Consider adding input validation

The getFilePath function should validate the input filename to prevent directory traversal attacks.

 export function getFilePath(filename: string) {
+  if (!filename || filename.includes('..')) {
+    throw new Error('Invalid filename');
+  }
   return path.join(fixtures, filename);
 }
.github/workflows/nodejs.yml (1)

14-15: Consider using more specific version ranges

While the Node.js versions align with the requirement to support 18.19.0+, consider:

  • Specifying the exact patch versions for better reproducibility
  • Adding comments to indicate LTS versions
       os: 'ubuntu-latest'
-      version: '18.19.0, 18, 20, 22'
+      version: '18.19.0, 18.19.1, 20.11.0, 22.0.0' # 18 and 20 are LTS versions
src/config/config.default.ts (2)

13-14: Consider using path.resolve() for more robust path handling

While path.join() works, using path.resolve() would ensure absolute paths and handle edge cases better across different platforms.

-      default: path.join(getSourceDirname(), 'lib', 'event-sources', 'default'),
-      development: path.join(getSourceDirname(), 'lib', 'event-sources', 'development'),
+      default: path.resolve(getSourceDirname(), 'lib', 'event-sources', 'default'),
+      development: path.resolve(getSourceDirname(), 'lib', 'event-sources', 'development'),

5-9: Enhance TypeScript documentation with interface definitions

Consider adding an interface definition for the watcher config structure to improve type safety and documentation.

interface WatcherConfig {
  type: 'default' | 'development';
  eventSources: {
    default: string;
    development: string;
  };
}
src/lib/event-sources/default.ts (2)

7-7: Consider extracting message strings into constants

String literals in code can lead to maintenance issues. Consider extracting these messages into named constants.

const MESSAGES = {
  WATCHER_INACTIVE: '[@eggjs/watcher] defaultEventSource watcher will NOT take effect',
  WATCH_NOOP: '[@eggjs/watcher] using defaultEventSource watcher.watch() does NOTHING',
  UNWATCH_NOOP: '[@eggjs/watcher] using defaultEventSource watcher.unwatch() does NOTHING',
} as const;

11-13: Add TypeScript return types to methods

Methods should have explicit return type annotations for better type safety.

-  watch() {
+  watch(): void {
     this.emit('info', '[@eggjs/watcher] using defaultEventSource watcher.watch() does NOTHING');
   }

-  unwatch() {
+  unwatch(): void {
     this.emit('info', '[@eggjs/watcher] using defaultEventSource watcher.unwatch() does NOTHING');
   }

Also applies to: 15-17

src/lib/utils.ts (2)

16-18: Replace @ts-ignore with more specific @ts-expect-error

Using @ts-ignore suppresses all errors, while @ts-expect-error requires you to be explicit about the expected error.

-  // eslint-disable-next-line @typescript-eslint/ban-ts-comment
-  // @ts-ignore
+  // @ts-expect-error import.meta.url is only available in ESM

11-20: Consider caching the dirname result

The getSourceDirname function could cache its result since the dirname won't change during runtime.

let cachedDirname: string | undefined;

export function getSourceDirname(): string {
  if (cachedDirname) return cachedDirname;
  
  if (typeof __dirname === 'string') {
    cachedDirname = path.dirname(__dirname);
    return cachedDirname;
  }

  // @ts-expect-error import.meta.url is only available in ESM
  const __filename = fileURLToPath(import.meta.url);
  cachedDirname = path.dirname(path.dirname(__filename));
  return cachedDirname;
}
src/lib/types.ts (3)

1-2: Use explicit imports from node:fs

Consider explicitly importing only the required types from node:fs for better tree-shaking.

-import type { WatchEventType, Stats } from 'node:fs';
+import type { WatchEventType, Stats } from 'node:fs/promises';

4-10: Consider making the watcher type more specific

The type property in WatcherConfig could benefit from a union type of supported watcher types for better type safety.

export interface WatcherConfig {
  watcher: {
-    type: string;
+    type: 'default' | 'development';
    eventSources: Record<string, string>;
  };
  [key: string]: Record<string, any>;
}

21-35: Consider splitting module augmentation into a separate file

The module augmentation for 'egg' is quite extensive. Consider moving it to a separate types file (e.g., egg.d.ts) to maintain better separation of concerns.

test/fixtures/apps/watcher-development-app/app/router.js (2)

9-12: Add type information to callback function

Since the project is using TypeScript, consider adding type information to the callback function.

-  function callback(info) {
+  function callback(info: { event: string; path: string }) {
     console.log('got change %j', info);
     fileChangeCount++;
   }

Line range hint 14-47: Consider using async/await consistently

The code mixes Promise-based and async/await syntax. Consider using async/await consistently throughout the router handlers.

-  app.get('/agent-watch', async ctx => {
-    app.messenger.broadcast('agent-watch');
-    ctx.body = await new Promise(function(resolve) {
-      app.messenger.on('agent-watch-success', function(msg) {
-        resolve(msg);
-      });
-    });
-  });
+  app.get('/agent-watch', async ctx => {
+    const messagePromise = new Promise(resolve => {
+      app.messenger.once('agent-watch-success', resolve);
+    });
+    app.messenger.broadcast('agent-watch');
+    ctx.body = await messagePromise;
+  });
test/watcher.test.ts (1)

8-8: Consider using optional chaining.

Replace app && app.close() with app?.close() for more concise code.

-  afterEach(() => app && app.close());
+  afterEach(() => app?.close());
🧰 Tools
🪛 Biome (1.9.4)

[error] 8-8: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

src/lib/event-sources/development.ts (1)

86-91: Consider handling stat errors gracefully.

The statSync call might fail if the file is deleted between the event and the stat check.

-    const info = {
-      path: file,
-      event,
-      stat: fs.statSync(file, { throwIfNoEntry: false }),
-    } as ChangeInfo;
-    this.emit('change', info);
+    const stat = fs.statSync(file, { throwIfNoEntry: false });
+    if (stat || event === 'rename') {
+      const info = { path: file, event, stat } as ChangeInfo;
+      this.emit('change', info);
+    }
test/development_cluster.test.ts (2)

48-51: Consider using fs.promises API for file operations

Since we're modernizing the codebase and requiring Node.js 18.19.0+, consider using the promise-based fs API for better async flow control.

-fs.writeFileSync(file_path3, 'aaa');
-fs.mkdirSync(file_path4, { recursive: true });
-fs.rmdirSync(file_path4);
-fs.rmSync(file_path4, { force: true });
+await fs.promises.writeFile(file_path3, 'aaa');
+await fs.promises.mkdir(file_path4, { recursive: true });
+await fs.promises.rm(file_path4, { force: true });

Line range hint 63-81: Track unsubscribe implementation as a separate issue

The TODO comment about unsubscribe implementation should be tracked properly.

Would you like me to create a GitHub issue to track the unsubscribe implementation for the cluster-client?

test/development.test.ts (1)

84-85: Document why agent watcher test is skipped

The comment "not work on cluster message" needs more context. Consider adding a detailed explanation of why this test is skipped and what conditions would make it work.

-  // not work on cluster message
+  // TODO: Agent watcher tests are skipped in development mode because
+  // cluster messaging is not supported in this context. This should be
+  // tested in the integration test suite instead.
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 10f7971 and 47472ef.

📒 Files selected for processing (54)
  • .autod.conf.js (0 hunks)
  • .eslintrc (1 hunks)
  • .github/PULL_REQUEST_TEMPLATE.md (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)
  • .travis.yml (0 hunks)
  • README.md (5 hunks)
  • agent.js (0 hunks)
  • app.js (0 hunks)
  • appveyor.yml (0 hunks)
  • config/config.default.js (0 hunks)
  • config/config.local.js (0 hunks)
  • config/config.unittest.js (0 hunks)
  • lib/event-sources/default.js (0 hunks)
  • lib/event-sources/development.js (0 hunks)
  • lib/init.js (0 hunks)
  • lib/utils.js (0 hunks)
  • lib/watcher.js (0 hunks)
  • package.json (2 hunks)
  • src/agent.ts (1 hunks)
  • src/app.ts (1 hunks)
  • src/config/config.default.ts (1 hunks)
  • src/config/config.local.ts (1 hunks)
  • src/config/config.unittest.ts (1 hunks)
  • src/index.ts (1 hunks)
  • src/lib/boot.ts (1 hunks)
  • src/lib/event-sources/base.ts (1 hunks)
  • src/lib/event-sources/default.ts (1 hunks)
  • src/lib/event-sources/development.ts (1 hunks)
  • src/lib/event-sources/index.ts (1 hunks)
  • src/lib/types.ts (1 hunks)
  • src/lib/utils.ts (1 hunks)
  • src/lib/watcher.ts (1 hunks)
  • test/development.test.ts (1 hunks)
  • test/development_cluster.test.ts (6 hunks)
  • test/fixtures/apps/watcher-custom-event-source-fuzzy/plugins/egg-watcher-custom/config/config.default.js (0 hunks)
  • test/fixtures/apps/watcher-custom-event-source-fuzzy/plugins/egg-watcher-custom/custom.js (1 hunks)
  • test/fixtures/apps/watcher-custom-event-source/config/config.unittest.js (0 hunks)
  • test/fixtures/apps/watcher-custom-event-source/config/plugin.js (0 hunks)
  • test/fixtures/apps/watcher-custom-event-source/plugins/egg-watcher-custom/custom.js (1 hunks)
  • test/fixtures/apps/watcher-development-app/agent.js (1 hunks)
  • test/fixtures/apps/watcher-development-app/app/router.js (1 hunks)
  • test/fixtures/apps/watcher-development-app/config/config.unittest.js (0 hunks)
  • test/fixtures/apps/watcher-development-app/package.json (1 hunks)
  • test/fixtures/apps/watcher-development-app/tmp/t1/t2/t3/t4/tmp.txt (1 hunks)
  • test/fixtures/apps/watcher-type-default/app.js (0 hunks)
  • test/fixtures/apps/watcher-type-default/config/config.unittest.js (0 hunks)
  • test/utils.js (0 hunks)
  • test/utils.ts (1 hunks)
  • test/watcher.test.js (0 hunks)
  • test/watcher.test.ts (1 hunks)
  • tsconfig.json (1 hunks)
💤 Files with no reviewable changes (22)
  • test/fixtures/apps/watcher-custom-event-source-fuzzy/plugins/egg-watcher-custom/config/config.default.js
  • test/fixtures/apps/watcher-type-default/config/config.unittest.js
  • test/fixtures/apps/watcher-development-app/config/config.unittest.js
  • test/fixtures/apps/watcher-custom-event-source/config/plugin.js
  • config/config.local.js
  • test/fixtures/apps/watcher-custom-event-source/config/config.unittest.js
  • config/config.unittest.js
  • app.js
  • test/fixtures/apps/watcher-type-default/app.js
  • .github/PULL_REQUEST_TEMPLATE.md
  • lib/event-sources/default.js
  • .travis.yml
  • config/config.default.js
  • test/watcher.test.js
  • lib/init.js
  • agent.js
  • appveyor.yml
  • test/utils.js
  • lib/utils.js
  • lib/event-sources/development.js
  • lib/watcher.js
  • .autod.conf.js
✅ Files skipped from review due to trivial changes (12)
  • test/fixtures/apps/watcher-development-app/tmp/t1/t2/t3/t4/tmp.txt
  • test/fixtures/apps/watcher-development-app/package.json
  • src/config/config.unittest.ts
  • .gitignore
  • src/agent.ts
  • src/lib/event-sources/index.ts
  • src/app.ts
  • src/index.ts
  • src/config/config.local.ts
  • .github/workflows/release.yml
  • test/fixtures/apps/watcher-custom-event-source/plugins/egg-watcher-custom/custom.js
  • test/fixtures/apps/watcher-custom-event-source-fuzzy/plugins/egg-watcher-custom/custom.js
🧰 Additional context used
🪛 Biome (1.9.4)
test/watcher.test.ts

[error] 8-8: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🪛 LanguageTool
README.md

[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 2105 characters long)
Context: ...r [![NPM version][npm-image]][npm-url] Node.js CI [![Test coverage][codecov-image]][codecov-url] [![Known Vulnerabilities][snyk-image]][snyk-url] [![npm download][download-image]][download...

(EN_EXCESSIVE_EXCLAMATION)


[uncategorized] ~59-~59: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...cal(env is local). Once files on disk is modified it will emit a change event ...

(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)


[uncategorized] ~59-~59: A comma might be missing here.
Context: ...(env is local). Once files on disk is modified it will emit a change event immediate...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)


[uncategorized] ~64-~64: A comma might be missing here.
Context: ...om, eggPlugin.name: watcherCustom`). Firstly define our custom event source like thi...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)

🔇 Additional comments (15)
test/fixtures/apps/watcher-development-app/agent.js (2)

1-1: LGTM! Good use of the node: protocol prefix

Using the node: prefix for built-in modules is a best practice that helps prevent module naming conflicts and improves code clarity.


3-4: 🛠️ Refactor suggestion

Consider ESM compatibility for path resolution

While using path.join() with __dirname works well for CommonJS, it may cause issues when this module is imported as ESM, where __dirname is not available. Consider making this code dual-format compatible.

Here's a script to check if this file might be imported as ESM:

Consider this ESM-compatible alternative:

-const file_path1 = path.join(__dirname, 'tmp-agent.txt');
-const dir_path = path.join(__dirname, 'tmp-agent');
+import { fileURLToPath } from 'node:url';
+const __filename = fileURLToPath(import.meta.url);
+const __dirname = path.dirname(__filename);
+
+const file_path1 = path.join(__dirname, 'tmp-agent.txt');
+const dir_path = path.join(__dirname, 'tmp-agent');
src/lib/watcher.ts (2)

24-45: Verify dynamic import error handling.
When loading the event source module with “importModule”, consider handling or logging errors if the import fails. This ensures that any misconfiguration or missing module is promptly visible.


89-101: Validate path matching logic (#onChange).
If the “path” is quite large or has symbolic links, “isEqualOrParentPath” might produce false positives or negatives. Check potential edge cases involving link resolution or unusual file system structures.

.eslintrc (1)

2-5: Great approach for modular ESLint configuration.
The usage of multiple configs (“typescript” and “enforce-node-prefix”) can help keep rules organized and specialized. This is aligned with TypeScript best practices.

tsconfig.json (1)

2-10: Good selection of compiler options for modern Node versions.
Specifying “ES2022” and “NodeNext” is consistent with dropping older Node versions and focusing on ESM compliance. Strict type-checking is a plus.

test/utils.ts (1)

1-6: Well-structured ESM implementation!

Good practices observed:

  • Using 'node:' prefix for built-in modules
  • Proper ESM path handling with fileURLToPath
  • Clean TypeScript implementation
src/lib/event-sources/default.ts (1)

7-8: Verify Node.js 18.19.0+ compatibility with setImmediate

Since we're dropping support for Node.js versions < 18.19.0, let's verify that setImmediate is the best choice here.

package.json (3)

63-75: LGTM! Proper dual module system configuration.

The exports field correctly defines both ESM and CJS entry points with their corresponding type definitions, following Node.js best practices for dual module support.


42-55: Verify TypeScript build tooling configuration.

The development dependencies include necessary packages for TypeScript builds:

  • tshy/tshy-after for dual CJS/ESM builds
  • @eggjs/tsconfig for consistent TypeScript configuration
  • @arethetypeswrong/cli for type checking

35-35: Verify Node.js version compatibility.

The minimum Node.js version requirement of 18.19.0 aligns with the PR objective of dropping support for older versions. This version provides stable ESM support.

✅ Verification successful

Let me gather more information about Node.js version requirements across the codebase.


Let me check for any ESM-specific features or imports that would justify the Node.js version requirement.


Node.js version requirement is justified and properly aligned

The Node.js version requirement of >= 18.19.0 is appropriate because:

  • The package is configured as an ES Module (type: "module" in package.json)
  • Extensive use of ESM imports across multiple TypeScript files
  • Node.js 18.19.0+ provides stable ESM support and necessary TypeScript features
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify Node.js version compatibility in related packages
gh api graphql -f query='
{
  repository(owner: "eggjs", name: "egg") {
    packages(first: 10) {
      nodes {
        name
        versions(first: 1) {
          nodes {
            files(first: 10) {
              nodes {
                name
              }
            }
          }
        }
      }
    }
  }
}'

Length of output: 370


Script:

#!/bin/bash
# Check for Node.js version requirements in package.json files
fd package.json --exec cat {} | jq -r 'select(.engines?.node != null) | "File: \(.name // "package.json")\nNode version: \(.engines.node)"'

# Check for potential Node.js version specific features or APIs
rg -i "node.*version|engines.*node" --type js --type ts -C 2

# Look for any version checks or assertions
ast-grep --pattern 'process.version'

Length of output: 289


Script:

#!/bin/bash
# Check for ESM syntax and features
rg "import\s+.*\s+from\s+" --type js --type ts -l

# Check for package.json type field
fd package.json --exec jq -r 'select(.type != null) | "File: \(.name // "package.json")\nType: \(.type)"'

# Look for any .mjs files
fd -e mjs

# Check for import assertions or other modern import features
rg "import.*assert.*{.*}" --type js --type ts

Length of output: 688

test/watcher.test.ts (1)

1-4: LGTM! Proper ESM import syntax.

The import statements follow ESM best practices:

  • Using node: prefix for Node.js built-ins
  • Including .js extension in relative imports
src/lib/event-sources/development.ts (1)

1-5: LGTM! Proper ESM imports and type usage.

The imports follow ESM best practices and properly separate types for TypeScript.

test/development_cluster.test.ts (2)

1-5: LGTM: Modern import statements with node: protocol

The imports are correctly using the new node: protocol prefix and ESM import syntax, which aligns with the PR objective of supporting ESM modules.


16-21: Verify cluster mode configuration

The test setup uses mm.cluster() but the plugin configuration is commented out. This might affect the test's validity.

.github/workflows/pkg.pr.new.yml Show resolved Hide resolved
src/lib/utils.ts Show resolved Hide resolved
src/lib/boot.ts Show resolved Hide resolved
src/lib/boot.ts Show resolved Hide resolved
src/lib/event-sources/development.ts Outdated Show resolved Hide resolved
test/development.test.ts Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@fengmk2 fengmk2 merged commit c80fea0 into master Dec 18, 2024
18 checks passed
@fengmk2 fengmk2 deleted the egg-v4 branch December 18, 2024 12:53
fengmk2 pushed a commit that referenced this pull request Dec 18, 2024
[skip ci]

## 1.0.0 (2024-12-18)

### ⚠ BREAKING CHANGES

* drop Node.js < 18.19.0 support

part of eggjs/egg#3644

eggjs/egg#5257

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

## Summary by CodeRabbit

- **New Features**
- Introduced new configuration files for managing watcher settings in
different environments (default, local, unittest).
- Added a new `Boot` class to manage application lifecycle and watcher
initialization.
- Implemented `Watcher` class for monitoring file changes with event
handling.
- Added `DevelopmentEventSource` and `DefaultEventSource` classes for
specific event source management.

- **Bug Fixes**
- Enhanced path handling in various modules to ensure correct file
watching functionality.

- **Documentation**
	- Updated `README.md` with project name change and improved structure.

- **Tests**
- Introduced new unit tests for watcher functionality and refactored
existing test files to improve clarity and structure.

- **Chores**
- Removed deprecated configuration files and streamlined project
structure.
	- Updated TypeScript configuration for stricter type-checking.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

### Features

* [BREAK CHANGE]  use cluster-client ([#2](#2)) ([90a8b47](90a8b47))
* add event source event logs ([#9](#9)) ([b351795](b351795))
* pass custom options ([#4](#4)) ([cf9fcac](cf9fcac))
* support cjs and esm both by tshy ([#14](#14)) ([c80fea0](c80fea0))

### Bug Fixes

* should support watch one file multiple times ([#6](#6)) ([6d84e21](6d84e21))
* spell error on watcher.js ([#13](#13)) ([9ab2eed](9ab2eed))
fengmk2 pushed a commit that referenced this pull request Dec 18, 2024
[skip ci]

## [4.0.0](v3.1.1...v4.0.0) (2024-12-18)

### ⚠ BREAKING CHANGES

* drop Node.js < 18.19.0 support

part of eggjs/egg#3644

eggjs/egg#5257

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

## Summary by CodeRabbit

- **New Features**
- Introduced new configuration files for managing watcher settings in
different environments (default, local, unittest).
- Added a new `Boot` class to manage application lifecycle and watcher
initialization.
- Implemented `Watcher` class for monitoring file changes with event
handling.
- Added `DevelopmentEventSource` and `DefaultEventSource` classes for
specific event source management.

- **Bug Fixes**
- Enhanced path handling in various modules to ensure correct file
watching functionality.

- **Documentation**
	- Updated `README.md` with project name change and improved structure.

- **Tests**
- Introduced new unit tests for watcher functionality and refactored
existing test files to improve clarity and structure.

- **Chores**
- Removed deprecated configuration files and streamlined project
structure.
	- Updated TypeScript configuration for stricter type-checking.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

### Features

* support cjs and esm both by tshy ([#14](#14)) ([c80fea0](c80fea0))
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.

1 participant