-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces comprehensive changes to the Changes
Sequence DiagramsequenceDiagram
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
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected] |
There was a problem hiding this 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 multipleif
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 betweentryToResolveByDirnameFromPackage
andtryToResolveFromFile
for easier maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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.
commit: |
commit: |
There was a problem hiding this 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 maintainabilityThe 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
- The nullish coalescing operator usage
pkg?.type ?? (isESM ? 'module' : 'commonjs')
might not handle all edge cases correctly ifpkg.type
is an empty string.- 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 usageThe 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 organizationThe 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 handlingThe 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
📒 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.
Codecov ReportAttention: Patch coverage is
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. |
There was a problem hiding this 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 stabilityUsing
@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 processThe current setup is complex and potentially fragile:
- It first sets up Node.js 16
- Then uses a hardcoded nvm.sh path to switch to Node.js 14
- Uses
@master
for the GitHub ActionConsider 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:
- The package.json is configured as an ESM package (type: "module") with dual ESM/CommonJS exports
- The test files are using native ESM imports (e.g.,
import { strict as assert } from 'node:assert'
)- No explicit Node.js version requirements are specified in package.json
- 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:
- Rename the workflow file to
nodejs-16.yml
to match the actual Node.js version being used, or- 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 14Given 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 fiLength 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 fiLength 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 ofcjs/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 topkg.main
andpkg.module
, you’re now supportingtshy.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
📒 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.
There was a problem hiding this 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 issueNode.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.0Update 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.jsonThe 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 logicThe 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
📒 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
[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))
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores