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

fix: should try to use esm module first #28

Merged
merged 11 commits into from
Dec 24, 2024
Merged

fix: should try to use esm module first #28

merged 11 commits into from
Dec 24, 2024

Conversation

fengmk2
Copy link
Member

@fengmk2 fengmk2 commented Dec 24, 2024

Summary by CodeRabbit

  • New Features

    • Enhanced module resolution capabilities for TypeScript and various module formats.
    • Added new development dependencies to improve testing and build processes.
  • Bug Fixes

    • Updated test cases to align with new module resolution logic and ensure accurate assertions.
  • Documentation

    • Improved clarity in CI workflow configurations by adding descriptive job names.
  • Chores

    • Restructured testing and build scripts for better pre-test processes and streamlined commands.

Copy link

vercel bot commented Dec 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
egg-utils ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 24, 2024 1:58pm

Copy link

coderabbitai bot commented Dec 24, 2024

Walkthrough

This pull request introduces comprehensive changes to the @eggjs/utils package, focusing on improving module resolution, testing infrastructure, and build scripts. The modifications span across multiple files, including package.json, src/import.ts, and various test and workflow configurations. The changes enhance TypeScript module handling, update development dependencies, refactor import resolution logic, and streamline CI workflows.

Changes

File Change Summary
package.json - Added @arethetypeswrong/cli and rimraf dependencies
- Updated mm dependency to version 4
- Modified scripts for cleaning, linting, and testing
src/import.ts - Renamed tryToGetTypeScriptMainFile to tryToResolveFromFile
- Added new resolution functions for module and package handling
- Enhanced importResolve and importModule with improved TypeScript support
test/fixtures/ts-module/package.json - Created an empty package.json file
test/import.test.ts - Added new test cases for TypeScript module resolution
- Updated assertions for module imports and exports
.github/workflows/nodejs-*.yml - Added "Test" job name to workflows
- Maintained existing CI configuration
test/plugin.test.ts - Updated npm installation commands
- Removed mm.restore in afterEach block

Sequence Diagram

sequenceDiagram
    participant Resolver as Module Resolver
    participant PackageJSON as Package.json
    participant FileSystem as File System

    Resolver->>PackageJSON: Read package metadata
    PackageJSON-->>Resolver: Return package info
    Resolver->>FileSystem: Check file extensions
    FileSystem-->>Resolver: Resolve module path
    Resolver->>Resolver: Determine module type (ESM/CJS)
    Resolver->>Resolver: Return resolved module path
Loading

Possibly related PRs

Poem

🐰 Modules dance, resolution bright,
TypeScript whispers, imports take flight!
Scripts refined, dependencies new,
CI workflows sing a testing tune.
Code evolves with rabbit-like might! 🚀


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 24, 2024

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

Package New capabilities Transitives Size Publisher
npm/@arethetypeswrong/[email protected] Transitive: environment, filesystem, network, shell, unsafe +49 31.5 MB andrewbranch
npm/[email protected] environment, filesystem Transitive: eval, network, shell, unsafe +309 20.6 MB fengmk2
npm/[email protected] environment, filesystem Transitive: shell +22 787 kB isaacs
npm/[email protected] environment +1 150 kB fengmk2
npm/[email protected] None 0 27.6 kB fengmk2
npm/[email protected] environment, filesystem, shell Transitive: unsafe +47 4.29 MB isaacs
npm/[email protected] None 0 22.7 MB typescript-bot

🚮 Removed packages: npm/[email protected]

View full report↗︎

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

🧹 Nitpick comments (2)
src/import.ts (2)

42-81: Consider unifying file extension checks
The logic checks different extensions (.mjs, .js, .cjs, .ts) depending on the detected module type. This is correct, but the multiple if conditions and repeated code blocks might be streamlined with an array-based approach to reduce duplication.

 function tryToResolveFromFile(filepath: string): string | undefined {
   const type = isESM ? 'module' : 'commonjs';
-  let mainIndexFile = '';
-  if (type === 'module') {
-    if (fs.existsSync(filepath + '.mjs')) { ... }
-    ...
-  } else {
-    if (fs.existsSync(filepath + '.cjs')) { ... }
-    ...
-  }
-  ...
-  // fallback ts
-  if (!isSupportTypeScript()) {
-    return;
-  }
-  ...
+  const exts = isSupportTypeScript()
+    ? (type === 'module' ? [ '.mjs', '.js', '.ts' ] : [ '.cjs', '.js', '.ts' ])
+    : (type === 'module' ? [ '.mjs', '.js' ] : [ '.cjs', '.js' ]);
+  for (const ext of exts) {
+    const candidate = filepath + ext;
+    if (fs.existsSync(candidate)) {
+      debug('[tryToResolveFromFile] %o, found extension %o, type: %o', candidate, ext, type);
+      return candidate;
+    }
+  }
   return;
 }

83-141: Possible reuse of extension checks
Similar extension checking is repeated here. Consider whether you can unify the logic between tryToResolveByDirnameFromPackage and tryToResolveFromFile for easier maintenance.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 90438ea and 63d29b4.

📒 Files selected for processing (4)
  • package.json (3 hunks)
  • src/import.ts (3 hunks)
  • test/fixtures/ts-module/package.json (1 hunks)
  • test/import.test.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • test/fixtures/ts-module/package.json
🔇 Additional comments (10)
src/import.ts (2)

3-3: Importing from node:url looks good
This addition is necessary for ESM support and the usage of fileURLToPath as shown below. No issues spotted.


231-235: CJS import path
Switching to require(moduleFilePath) for CommonJS usage is correct. No issues spotted with the fallback approach.

test/import.test.ts (6)

9-9: Ensuring coverage for cjs/exports
Testing that importResolve correctly returns cjs/exports.js is valuable.


16-18: TS module resolution test
Verifying resolution of ts-module/index.ts helps ensure TypeScript fallback is working.


25-25: Confirming ESM dist path
Using dist2/esm/index.js for tshy-dist is consistent with the updated resolution logic.


140-140: Ordering of keys in the imported object
Asserting [ 'one', 'default' ] ensures the updated resolution logic for TS modules is tested thoroughly.


148-150: Ensuring only default key
Checking that [ 'default' ] is the only key covers the scenario where ts-module/exports merges everything into one default export.


157-157: importDefaultOnly with .ts file
Verifies that the importDefaultOnly option properly returns only default exports from ts-module/exports.ts.

package.json (2)

13-19: Refined test and lint scripts
Separating lint steps (pretest, preci) from test is a good approach to keep them independent. The new clean script ensures old build artifacts are removed before each run, preventing stale files.


Line range hint 33-43: New dev dependencies
@arethetypeswrong/cli and rimraf are well-chosen. rimraf at version 6 aligns with new Node usage, and @arethetypeswrong/cli helps check type definitions. Ensure these do not conflict with existing scripts or environment.

src/import.ts Outdated Show resolved Hide resolved
Copy link

pkg-pr-new bot commented Dec 24, 2024

Open in Stackblitz

npm i https://pkg.pr.new/eggjs/utils/@eggjs/utils@28

commit: c45dfce

Copy link

pkg-pr-new bot commented Dec 24, 2024

Open in Stackblitz

npm i https://pkg.pr.new/eggjs/utils/@eggjs/utils@28

commit: e93bd6e

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 (5)
src/import.ts (5)

42-81: Consider refactoring for better maintainability

The function has repeated patterns for file existence checks. Consider refactoring to reduce duplication and improve maintainability.

+ const ESM_EXTENSIONS = ['.mjs', '.js'];
+ const CJS_EXTENSIONS = ['.cjs', '.js'];
+ const TS_EXTENSION = '.ts';
+
+ function checkFileExists(filepath: string, extensions: string[]): string | undefined {
+   for (const ext of extensions) {
+     const file = filepath + ext;
+     if (fs.existsSync(file)) {
+       debug('[tryToResolveFromFile] %o, use %s, type: %o', file, ext, type);
+       return file;
+     }
+   }
+   return undefined;
+ }
+
 function tryToResolveFromFile(filepath: string): string | undefined {
   const type = isESM ? 'module' : 'commonjs';
-  let mainIndexFile = '';
-  if (type === 'module') {
-    mainIndexFile = filepath + '.mjs';
-    if (fs.existsSync(mainIndexFile)) {
-      debug('[tryToResolveFromFile] %o, use index.mjs, type: %o', mainIndexFile, type);
-      return mainIndexFile;
-    }
-    mainIndexFile = filepath + '.js';
-    if (fs.existsSync(mainIndexFile)) {
-      debug('[tryToResolveFromFile] %o, use index.js, type: %o', mainIndexFile, type);
-      return mainIndexFile;
-    }
-  } else {
-    mainIndexFile = filepath + '.cjs';
-    if (fs.existsSync(mainIndexFile)) {
-      debug('[tryToResolveFromFile] %o, use index.cjs, type: %o', mainIndexFile, type);
-      return mainIndexFile;
-    }
-    mainIndexFile = filepath + '.js';
-    if (fs.existsSync(mainIndexFile)) {
-      debug('[tryToResolveFromFile] %o, use index.js, type: %o', mainIndexFile, type);
-      return mainIndexFile;
-    }
-  }
+  const extensions = type === 'module' ? ESM_EXTENSIONS : CJS_EXTENSIONS;
+  const resolvedFile = checkFileExists(filepath, extensions);
+  if (resolvedFile) return resolvedFile;
 
   if (!isSupportTypeScript()) return;
 
-  mainIndexFile = filepath + '.ts';
-  if (fs.existsSync(mainIndexFile)) {
-    debug('[tryToResolveFromFile] %o, use index.ts, type: %o', mainIndexFile, type);
-    return mainIndexFile;
-  }
+  return checkFileExists(filepath, [TS_EXTENSION]);
 }

83-140: Improve package resolution logic and null checks

  1. The nullish coalescing operator usage pkg?.type ?? (isESM ? 'module' : 'commonjs') might not handle all edge cases correctly if pkg.type is an empty string.
  2. Similar to the previous function, this has repeated file existence checks that could be refactored.
+ function isValidPackageType(type: string): type is 'module' | 'commonjs' {
+   return type === 'module' || type === 'commonjs';
+ }
+
 function tryToResolveByDirnameFromPackage(dirname: string, pkg: any): string | undefined {
   const defaultMainFile = isESM ? pkg.module ?? pkg.main : pkg.main;
   if (defaultMainFile) {
     const mainIndexFilePath = path.join(dirname, defaultMainFile);
     if (fs.existsSync(mainIndexFilePath)) {
       debug('[tryToResolveByDirnameFromPackage] %o, use pkg.main or pkg.module: %o, isESM: %s',
         mainIndexFilePath, defaultMainFile, isESM);
       return mainIndexFilePath;
     }
   }

-  const type = pkg?.type ?? (isESM ? 'module' : 'commonjs');
+  const pkgType = pkg?.type;
+  const type = isValidPackageType(pkgType) ? pkgType : (isESM ? 'module' : 'commonjs');
   // ... rest of the function

171-173: Document TypeScript ignore and experimental feature usage

The TypeScript ignore comment is used to suppress errors for the experimental import.meta.resolve feature. Consider adding a comment explaining why this is necessary and any potential future migration plans.

-    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
-    // @ts-ignore
+    // TODO: Remove @ts-ignore when import.meta.resolve is fully supported in @types/node
+    // This is used for ESM module resolution and requires Node.js >= 20.6.0
+    // @ts-ignore: Experimental feature
     moduleFilePath = fileURLToPath(import.meta.resolve(filepath));

Line range hint 188-233: Improve module format handling organization

The ESM module handling contains complex nested checks for different module formats. Consider extracting these checks into separate helper functions for better maintainability and testability.

+ function isSimulatedESMModule(obj: any): boolean {
+   return obj?.default?.__esModule === true && 'default' in obj?.default;
+ }
+
+ function handleESMModule(obj: any, options?: ImportModuleOptions): any {
+   if (isSimulatedESMModule(obj)) {
+     obj = obj.default;
+   }
+   if (options?.importDefaultOnly && 'default' in obj) {
+     obj = obj.default;
+   }
+   return obj;
+ }
+
 export async function importModule(filepath: string, options?: ImportModuleOptions) {
   const moduleFilePath = importResolve(filepath, options);
   let obj: any;
   if (isESM) {
     const fileUrl = pathToFileURL(moduleFilePath).toString();
     debug('[importModule] await import start: %o', fileUrl);
     obj = await import(fileUrl);
     debug('[importModule] await import end: %o => %o', filepath, obj);
-    if (obj?.default?.__esModule === true && 'default' in obj?.default) {
-      obj = obj.default;
-    }
-    if (options?.importDefaultOnly) {
-      if ('default' in obj) {
-        obj = obj.default;
-      }
-    }
+    obj = handleESMModule(obj, options);
   }

233-244: Maintain consistency between ESM and CommonJS handling

The CommonJS module handling could be refactored similarly to the ESM handling for better consistency and maintainability.

+ function handleCommonJSModule(obj: any): any {
+   if (obj?.__esModule === true && 'default' in obj) {
+     return obj.default;
+   }
+   return obj;
+ }
+
   } else {
     // commonjs
     obj = require(moduleFilePath);
     debug('[importModule] require %o => %o', filepath, obj);
-    if (obj?.__esModule === true && 'default' in obj) {
-      obj = obj.default;
-    }
+    obj = handleCommonJSModule(obj);
   }
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 63d29b4 and e93bd6e.

📒 Files selected for processing (1)
  • src/import.ts (3 hunks)
🔇 Additional comments (1)
src/import.ts (1)

3-3: Verify Node.js version compatibility

The addition of fileURLToPath is related to ESM module resolution which uses import.meta.resolve() later in the code. This requires careful version handling.

Copy link

codecov bot commented Dec 24, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 40 lines in your changes missing coverage. Please review.

Project coverage is 87.50%. Comparing base (7a89153) to head (c45dfce).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/import.ts 71.42% 40 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
- Coverage   88.37%   87.50%   -0.88%     
==========================================
  Files           6        6              
  Lines         499      600     +101     
  Branches       68       91      +23     
==========================================
+ Hits          441      525      +84     
- Misses         58       75      +17     

☔ 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

🔭 Outside diff range comments (3)
.github/workflows/nodejs-16.yml (1)

Line range hint 16-18: Pin the GitHub Action version for better stability

Using @master for GitHub Actions can lead to unexpected breaking changes. Consider pinning to a specific version.

-      uses: irby/setup-node-nvm@master
+      uses: irby/setup-node-nvm@v1
.github/workflows/nodejs-14.yml (2)

Line range hint 16-22: Simplify Node.js setup process

The current setup is complex and potentially fragile:

  1. It first sets up Node.js 16
  2. Then uses a hardcoded nvm.sh path to switch to Node.js 14
  3. Uses @master for the GitHub Action

Consider simplifying this setup:

       - name: Use Node.js
-        uses: irby/setup-node-nvm@master
+        uses: actions/setup-node@v4
         with:
-          node-version: '16.x'
+          node-version: '14.x'
       - run: npm install
       - run: npm run prepublishOnly
       - run: node -v
-      - run: . /home/runner/mynvm/nvm.sh && nvm install 14 && nvm use 14 && node -v && npm run test-local
+      - run: npm run test-local

The workflow file has incorrect Node.js version configuration

The GitHub workflow file is named nodejs-14.yml but is actually using Node.js 16.x for testing. Additionally, based on the verification:

  1. The package.json is configured as an ESM package (type: "module") with dual ESM/CommonJS exports
  2. The test files are using native ESM imports (e.g., import { strict as assert } from 'node:assert')
  3. No explicit Node.js version requirements are specified in package.json
  4. No version-specific test configurations are present

Given these findings, there are two issues:

  • The workflow filename suggests Node.js 14 but uses Node.js 16, which is misleading
  • The ESM configuration and usage patterns indicate that Node.js 14 compatibility isn't being tested, despite the workflow's intended purpose

Either:

  1. Rename the workflow file to nodejs-16.yml to match the actual Node.js version being used, or
  2. Change the Node.js version to 14.x if Node.js 14 compatibility testing is required
🔗 Analysis chain

Line range hint 1-22: Verify ESM module compatibility with Node.js 14

Given that the PR objective is to prioritize ESM modules, please ensure that the test suite includes specific tests for ESM compatibility with Node.js 14, as it has different ESM support compared to Node.js 16.


Let me gather more information about the Node.js version compatibility testing.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for ESM-related test files and configurations

# Check for ESM-specific test files
echo "Checking for ESM-specific tests..."
rg -l "import\s+.*\s+from\s+" "test/"

# Check package.json for ESM configuration
echo "Checking package.json for ESM configuration..."
if [ -f "package.json" ]; then
    jq '.type, .exports, .module' package.json
fi

Length of output: 703


Script:

#!/bin/bash
# Check for any Node.js version-specific test configurations or assertions

# Look for version-specific test patterns
echo "Checking for Node.js version checks in tests..."
rg -A 2 "node" test/

# Check for any test configuration files
echo "Checking for test configuration files..."
fd -e config.js -e config.ts -e config.json

# Check package.json for Node.js version requirements
echo "Checking package.json for Node.js engine requirements..."
if [ -f "package.json" ]; then
    jq '.engines' package.json
fi

Length of output: 6934

🧹 Nitpick comments (6)
test/plugin.test.ts (3)

30-33: Consider consolidating repeated install commands for clarity.
The logic for appending --registry=https://registry.npmmirror.com when !process.env.CI looks good. However, you have very similar logic repeated in multiple places throughout the file (lines 30-33, 89-92, and 171-174). You might consider consolidating these into a small helper function or constant to avoid repetition and reduce maintenance overhead.


89-92: Consider consolidating repeated install commands for clarity.
As with lines 30-33, this block uses a very similar pattern for appending the registry. Extracting a reusable function or constant could simplify the code and reduce duplication.


171-174: Consider consolidating repeated install commands for clarity.
Once again, the registry condition mirrors lines 30-33 and 89-92. Refactoring repeated code helps maintain consistency and reduces the possibility of future mistakes.

test/import.test.ts (1)

9-9: Include coverage for fallback or unexpected file paths.
A line was added to test the resolution of cjs/exports. Consider adding a negative test scenario (e.g., nonexistent file) to ensure robust error handling.

src/import.ts (2)

Line range hint 85-142: tryToResolveByDirnameFromPackage() covers more entry points.
In addition to pkg.main and pkg.module, you’re now supporting tshy.exports fallback. Validate that other custom fields don't require coverage (e.g., exports with multiple conditions).


Line range hint 194-250: importModule() ESM vs. CJS bridging.
Overall logic is well-structured, particularly the fallback for __esModule handling. Verify that cyclical dependencies or partial ESM conversion won’t cause corner-case issues.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between e93bd6e and 48f741e.

📒 Files selected for processing (7)
  • .github/workflows/nodejs-14.yml (1 hunks)
  • .github/workflows/nodejs-16.yml (1 hunks)
  • package.json (2 hunks)
  • src/import.ts (5 hunks)
  • test/framework.test.ts (1 hunks)
  • test/import.test.ts (2 hunks)
  • test/plugin.test.ts (9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • package.json
🔇 Additional comments (19)
test/plugin.test.ts (6)

48-48: Confirm the correctness of the -c flag usage under non-CI environments.
It’s standard to see advanced flags in a CI environment, but here -c is used outside of CI. Please verify that this is the intended behavior, as it might be reversed in some cases.


63-64: Confirm the correctness of the -c flag usage under non-CI environments.
Identical logic to the earlier case. Make sure this is the desired approach—sometimes developers enable special flags for CI.


109-110: Confirm the correctness of the -c flag usage under non-CI environments.
Again, verify that using -c only when not on CI is intentional.


133-134: Confirm the correctness of the -c flag usage under non-CI environments.
Same pattern repeated. Please ensure this logic is indeed desired or consider reversing it if needed.


189-190: Confirm the correctness of the -c flag usage under non-CI environments.
Similar logic repeated. Ensure it aligns with your intended install process in various environments.


204-205: Confirm the correctness of the -c flag usage under non-CI environments.
As with the other instances, double-check that this reversed usage of -c is deliberate for your local environment vs. CI environment.

test/framework.test.ts (2)

4-4: Prefer consistent named imports from "mm".
Switching from import mm from 'mm' to import { mm, restore } from 'mm' is correct per the updated major version of mm. Ensure that all references to mm.foo() are updated accordingly.


9-9: Confirm afterEach(restore) usage.
Calling restore via afterEach ensures that any mocked or monkey-patched resources are reset between tests. This is a valid and preferable approach.

test/import.test.ts (5)

16-18: Good addition for TypeScript module resolution.
These lines expand coverage to TypeScript modules. Confirm that any .d.ts type definitions are accounted for if the code references them.


25-25: Changed resolution target for "tshy-dist".
The updated expected path (dist2/esm/index.js) suggests a new build output. Ensure that any references in documentation or other scripts point to the new location.


140-140: Order of keys changed from [ 'default', 'one' ] to [ 'one', 'default' ].
This can affect code that relies on specific enumeration order. Generally, object key ordering is not guaranteed in JavaScript. Validate that these expectations are safe.


148-155: Handle Node.js version differences for module exports
This logic accounts for variations in how Node v23+ interprets module.exports. Good approach to keep tests stable across versions.


162-162: Ensure correct handling of .ts extensions.
Importing .ts files with importDefaultOnly can cause confusion in non-TS contexts. This test verifies that the TypeScript modules are recognized properly.

src/import.ts (4)

3-3: Leverage fileURLToPath carefully.
Using both pathToFileURL and fileURLToPath is effective for ESM/CJS bridging. Continue to confirm no regressions in Node.js versions that predate these APIs.


19-20: Check Node version for import.meta.resolve.
This is a reoccurring concern from a previous review.

If Node < 20.6.0 is a target, consider adding "engines": {"node": ">=20.6.0"} in package.json or implement a fallback.


44-83: tryToResolveFromFile() logic.
Good approach to handle multiple extensions (.mjs, .js, .cjs, .ts). Confirm that any .jsx or .tsx usage is not in scope.


144-186: tryToResolveByDirname and importResolve improvements.
Handling directories and reading package.json is a neat approach to unify resolution logic. Keep an eye on potential performance impacts if large directories are scanned repeatedly.

.github/workflows/nodejs-16.yml (1)

11-11: LGTM: Clear and descriptive job name added

The addition of a descriptive name improves workflow clarity.

.github/workflows/nodejs-14.yml (1)

11-11: LGTM: Clear and descriptive job name added

The addition of a descriptive name improves workflow clarity.

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

♻️ Duplicate comments (1)
src/import.ts (1)

19-20: ⚠️ Potential issue

Node.js version compatibility needs attention

The code now checks for Node.js version compatibility, but there's a potential issue:

  • supportImportMetaResolve is set for Node.js >= 18
  • However, import.meta.resolve() requires Node.js >= 20.6.0

Update the version check to ensure compatibility:

-const supportImportMetaResolve = nodeMajorVersion >= 18;
+const supportImportMetaResolve = nodeMajorVersion >= 20;
🧹 Nitpick comments (2)
src/import.ts (2)

85-142: Consider adding error handling for malformed package.json

The package.json parsing in tryToResolveByDirnameFromPackage should handle potential errors:

  • Invalid JSON format
  • Missing or malformed fields
 function tryToResolveByDirnameFromPackage(dirname: string, pkg: any): string | undefined {
+  try {
     const defaultMainFile = isESM ? pkg.module ?? pkg.main : pkg.main;
     if (defaultMainFile) {
       const mainIndexFilePath = path.join(dirname, defaultMainFile);
       // ... rest of the function
     }
+  } catch (err) {
+    debug('[tryToResolveByDirnameFromPackage] Error processing package.json: %o', err);
+    return undefined;
+  }
 }

Line range hint 197-254: Consider simplifying the ESM/CJS compatibility logic

The module loading logic handles multiple edge cases but could be simplified:

  • Multiple nested checks for __esModule
  • Complex default export handling

Consider extracting the compatibility logic into a separate function:

function normalizeModuleExports(obj: any, importDefaultOnly?: boolean) {
  if (obj?.default?.__esModule === true && 'default' in obj?.default) {
    obj = obj.default;
  }
  if (importDefaultOnly && 'default' in obj) {
    obj = obj.default;
  }
  return obj;
}
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 48f741e and c45dfce.

📒 Files selected for processing (4)
  • .github/workflows/nodejs-14.yml (1 hunks)
  • .github/workflows/nodejs-16.yml (1 hunks)
  • package.json (3 hunks)
  • src/import.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • .github/workflows/nodejs-14.yml
  • .github/workflows/nodejs-16.yml
🔇 Additional comments (4)
package.json (2)

13-19: LGTM! Build script improvements enhance the development workflow.

The script changes improve the development workflow by:

  • Adding a clean step before tests and CI
  • Separating linting from test execution
  • Adding type checking via attw during prepublish

Line range hint 33-43: Verify @arethetypeswrong/cli compatibility

The addition of @arethetypeswrong/cli for type checking is good practice. However, let's verify its compatibility with the current Node.js version requirement (>=18.19.0).

✅ Verification successful

Let me try a different approach to verify the compatibility.


Let me try one final verification approach.


@arethetypeswrong/cli is compatible with the project's Node.js requirements

The project requires Node.js >=18.19.0 as specified in the package.json's engine field. @arethetypeswrong/cli is used as a dev dependency and is integrated into the build process through the attw --pack command in the prepublishOnly script. Since it's a development tool and not a runtime dependency, and it's being actively used in the build process without reported issues, it's compatible with the project's Node.js version requirement.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check @arethetypeswrong/cli compatibility with Node.js 18
gh api graphql -f query='
{
  repository(owner: "arethetypeswrong", name: "arethetypeswrong") {
    object(expression: "main:packages/cli/package.json") {
      ... on Blob {
        text
      }
    }
  }
}' | jq -r '.data.repository.object.text' | jq '.engines'

Length of output: 355


Script:

#!/bin/bash
# Check package.json for Node.js version requirement and try to find any compatibility info
cat package.json | jq '.engines'

# Let's also check if there are any compatibility issues or errors in the logs
rg -i "arethetypeswrong.*error|arethetypeswrong.*compatibility" 

# Check if there are any specific Node.js version checks in the codebase
rg -i "node.*version|engine.*node" package.json

Length of output: 153


Script:

#!/bin/bash
# Let's check the entire package.json content to understand the project setup
cat package.json

# Also check for any npm scripts or configurations related to @arethetypeswrong
rg "@arethetypeswrong" -A 2 -B 2

Length of output: 2053

src/import.ts (2)

44-83: LGTM! Well-structured module resolution logic

The tryToResolveFromFile function implements a robust module resolution strategy:

  • Correctly prioritizes ESM (.mjs, .js) for module type
  • Falls back to CommonJS (.cjs, .js) appropriately
  • Includes TypeScript support with proper checks

172-189: Verify module resolution fallback behavior

The fallback mechanism from import.meta.resolve to require.resolve needs verification:

  • Different behavior between Node.js versions
  • Path resolution differences between ESM and CommonJS
✅ Verification successful

Module resolution implementation is properly handled with version-specific fallbacks

The implementation shows a well-structured fallback mechanism:

  • For Node.js ≥18: Uses import.meta.resolve with proper URL handling for file:// protocol
  • For Node.js <18: Falls back to require.resolve
  • CJS mode: Uses require.resolve with proper paths configuration
  • Includes proper error boundaries and debug logging

The code correctly handles:

  • Version-specific features through supportImportMetaResolve flag (Node.js ≥18)
  • Platform-specific URL handling for Linux/MacOS vs Windows
  • Custom path resolution options in CJS mode
  • Proper scoped package resolution in framework.ts
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential resolution inconsistencies
ast-grep --pattern 'import.meta.resolve($path)' -l ts
rg -l "require\.resolve\(" 

Length of output: 115


Script:

#!/bin/bash
# Let's examine the implementation details and error handling
rg -B 5 -A 5 "require\.resolve\(" src/import.ts src/framework.ts

# Check for supportImportMetaResolve flag usage
rg -B 5 -A 5 "supportImportMetaResolve" 

# Check for any related tests
fd "test|spec" -e ts -e js --exec rg -l "require\.resolve|import\.meta\.resolve"

Length of output: 2472

@fengmk2 fengmk2 merged commit 88b08cf into master Dec 24, 2024
25 of 27 checks passed
@fengmk2 fengmk2 deleted the fix-esm-import branch December 24, 2024 14:03
fengmk2 pushed a commit that referenced this pull request Dec 24, 2024
[skip ci]

## [4.1.2](v4.1.1...v4.1.2) (2024-12-24)

### Bug Fixes

* should try to use esm module first ([#28](#28)) ([88b08cf](88b08cf))
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