Add module resolver#17
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThis PR adds a centralized FileExtensions unit and a pluggable TGocciaModuleResolver, integrates resolver support into Engine and Interpreter (aliasing, extension probing, index resolution, global modules), extends extension handling to include .mjs, and adds tests and docs for extensionless imports and index resolution. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant Engine as TGocciaEngine
participant Interpreter as TGocciaInterpreter
participant Resolver as TGocciaModuleResolver
participant FS as File System
Client->>Engine: Create(entry, source, globals, resolver?)
Engine->>Interpreter: Construct + assign Resolver
Client->>Interpreter: Request module(specifier, importerPath)
Interpreter->>Interpreter: Check GlobalModules cache
alt Found in GlobalModules
Interpreter-->>Client: Return cached global module
else
Interpreter->>Resolver: Resolve(specifier, importerPath)
Resolver->>Resolver: ApplyAliases(specifier)
Resolver->>FS: Probe exact path -> extensions -> index files
FS-->>Resolver: Resolved path / not found
Resolver-->>Interpreter: ResolvedPath or raise EGocciaModuleNotFound
Interpreter->>FS: Read ResolvedPath
FS-->>Interpreter: File contents
Interpreter->>Interpreter: Parse/transform/execute & cache
Interpreter-->>Client: Module exports
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/language/modules/mjs-import.js (1)
1-11: LGTM!Well-structured test file covering the essential happy paths for
.mjsmodule imports. Tests both value and function exports appropriately.Consider adding edge case tests in the future, such as:
- Importing from
.mjswith extensionless path (e.g.,"./helpers/mjs-module")- Re-exporting from
.mjsmodules🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/language/modules/mjs-import.js` around lines 1 - 11, Add two additional edge-case tests to the existing suite in the mjs module tests: one that imports from the same module using an extensionless path (use the existing test file mjs-import.js and the module name "./helpers/mjs-module" to verify mjsValue and mjsAdd still work) and another that exercises re-exports (create or import a re-exporting module that re-exports the .mjs exports and add tests asserting the re-exported value and function behave identically); update or add test cases near the existing describe("mjs module import") block referencing the symbols mjsValue and mjsAdd to validate these scenarios.units/Goccia.Modules.Resolver.pas (1)
57-87: Alias matching uses first-match semantics.The alias matching iterates through the dictionary and exits on the first match. Since
TDictionaryiteration order is not guaranteed, if multiple aliases could match the same prefix (e.g.,@/and@/sub/), the behavior is nondeterministic.This is likely fine if aliases are intended to be non-overlapping prefixes, but worth documenting or considering longest-prefix-match if overlapping aliases are a use case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Modules.Resolver.pas` around lines 57 - 87, The current HasAlias and ApplyAliases use first-match semantics over FAliases which is nondeterministic because TDictionary iteration order is not guaranteed; modify both functions to perform a longest-prefix match instead: iterate all pairs in FAliases, track the Pair with the longest Pair.Key that matches the start of AModulePath (or Result), then after the loop apply the replacement using that best match (using FBaseDirectory the same way as now) and return accordingly; update HasAlias to return True only if any match was found (based on the longest-match check) so behavior is deterministic when overlapping prefixes (e.g., '@/' vs '@/sub/') exist.units/Goccia.Interpreter.pas (1)
142-154: Good defensive error handling with one potential gap.The global module lookup and resolver integration is well implemented. The check for an unassigned resolver (Lines 148-150) provides a clear error message.
However, the exception handler at Lines 151-154 only catches
EGocciaModuleNotFound. IfFResolver.Resolvethrows a different exception type (e.g., I/O errors fromFileExists), it would propagate unwrapped. Consider whether a broader catch is needed or if this is intentional (letting I/O errors bubble up directly).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Interpreter.pas` around lines 142 - 154, The except block currently only handles EGocciaModuleNotFound, so other exceptions thrown by FResolver.Resolve (e.g., IO errors) will escape unwrapped; update the handler around the FResolver.Resolve call so it catches Exception (or adds a separate except for Exception) and rethrows a TGocciaRuntimeError with the original exception's message and context (preserving AImportingFilePath) rather than letting non-EGocciaModuleNotFound exceptions propagate; reference FGlobalModules, FResolver.Resolve, EGocciaModuleNotFound and TGocciaRuntimeError when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/language/modules/index-import.js`:
- Around line 1-11: Add two new tests inside the existing "index file
resolution" describe: one edge-case that imports from a directory containing
multiple index candidates (e.g., a ./helpers/multi-index fixture) and asserts
the resolver picks the correct index file by expecting a specific exported value
(similar to how indexValue/indexDouble are used), and one error-case that
attempts to import from a directory with no index file (e.g.,
./helpers/missing-index) and asserts the import/require throws a resolver error
(use expect(() => require(...)).toThrow() or the equivalent dynamic-import
rejection). Ensure you add or update helper fixtures under ./helpers
(multi-index and missing-index) to reproduce the scenarios and keep the tests
focused only on index resolution.
---
Nitpick comments:
In `@tests/language/modules/mjs-import.js`:
- Around line 1-11: Add two additional edge-case tests to the existing suite in
the mjs module tests: one that imports from the same module using an
extensionless path (use the existing test file mjs-import.js and the module name
"./helpers/mjs-module" to verify mjsValue and mjsAdd still work) and another
that exercises re-exports (create or import a re-exporting module that
re-exports the .mjs exports and add tests asserting the re-exported value and
function behave identically); update or add test cases near the existing
describe("mjs module import") block referencing the symbols mjsValue and mjsAdd
to validate these scenarios.
In `@units/Goccia.Interpreter.pas`:
- Around line 142-154: The except block currently only handles
EGocciaModuleNotFound, so other exceptions thrown by FResolver.Resolve (e.g., IO
errors) will escape unwrapped; update the handler around the FResolver.Resolve
call so it catches Exception (or adds a separate except for Exception) and
rethrows a TGocciaRuntimeError with the original exception's message and context
(preserving AImportingFilePath) rather than letting non-EGocciaModuleNotFound
exceptions propagate; reference FGlobalModules, FResolver.Resolve,
EGocciaModuleNotFound and TGocciaRuntimeError when making the change.
In `@units/Goccia.Modules.Resolver.pas`:
- Around line 57-87: The current HasAlias and ApplyAliases use first-match
semantics over FAliases which is nondeterministic because TDictionary iteration
order is not guaranteed; modify both functions to perform a longest-prefix match
instead: iterate all pairs in FAliases, track the Pair with the longest Pair.Key
that matches the start of AModulePath (or Result), then after the loop apply the
replacement using that best match (using FBaseDirectory the same way as now) and
return accordingly; update HasAlias to return True only if any match was found
(based on the longest-match check) so behavior is deterministic when overlapping
prefixes (e.g., '@/' vs '@/sub/') exist.
Comment on lines
+1
to
+11
| import { indexValue, indexDouble } from "./helpers/indexed-module"; | ||
|
|
||
| describe("index file resolution", () => { | ||
| test("import from directory resolves to index.js", () => { | ||
| expect(indexValue).toBe("from-index"); | ||
| }); | ||
|
|
||
| test("import function from index module", () => { | ||
| expect(indexDouble(21)).toBe(42); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add edge/error coverage for index resolution.
This file only tests happy paths. Please add at least one edge case (e.g., directory with multiple candidate index extensions) and one error case (e.g., missing index file should raise a resolver error) while keeping the focus on index resolution. As per coding guidelines, "Each test file should focus on a single concern. Tests should cover happy paths, edge cases, and error cases."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/language/modules/index-import.js` around lines 1 - 11, Add two new
tests inside the existing "index file resolution" describe: one edge-case that
imports from a directory containing multiple index candidates (e.g., a
./helpers/multi-index fixture) and asserts the resolver picks the correct index
file by expecting a specific exported value (similar to how
indexValue/indexDouble are used), and one error-case that attempts to import
from a directory with no index file (e.g., ./helpers/missing-index) and asserts
the import/require throws a resolver error (use expect(() =>
require(...)).toThrow() or the equivalent dynamic-import rejection). Ensure you
add or update helper fixtures under ./helpers (multi-index and missing-index) to
reproduce the scenarios and keep the tests focused only on index resolution.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Modules.Resolver.pas`:
- Around line 93-99: The code treats only '/' as an absolute path, which breaks
on Windows; in ApplyAliases and Resolve replace the single-char '/' test with a
platform-aware absolute path check that (1) ensures Replacement is long enough
then tests for Windows drive-letter pattern (e.g., Replacement[2] = ':') and (2)
tests for UNC paths (Copy(Replacement,1,2) = '\\') in addition to the existing
Unix '/' check; update the conditional around Result assignment in ApplyAliases
(function ApplyAliases, variable Replacement) and the similar conditional in
Resolve so both branches correctly detect absolute paths across platforms
without adding external dependencies.
Contributor
Benchmark Results134 benchmarks · 134 unchanged · avg +2.3% arrays.js — 11 unchanged · avg +2.0%
classes.js — 10 unchanged · avg +3.3%
closures.js — 11 unchanged · avg +2.6%
collections.js — 12 unchanged · avg +2.0%
destructuring.js — 14 unchanged · avg +3.4%
fibonacci.js — 3 unchanged · avg +2.3%
json.js — 11 unchanged · avg -0.1%
jsx.jsx — 21 unchanged · avg +2.0%
numbers.js — 11 unchanged · avg +1.8%
objects.js — 7 unchanged · avg +2.1%
promises.js — 12 unchanged · avg +2.0%
strings.js — 11 unchanged · avg +3.9%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
Merged
This was referenced Mar 23, 2026
This was referenced Mar 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Code-style
Documentation
Tests