feat: windows support#663
Conversation
WalkthroughThe changes introduce systematic path normalization and platform-aware path handling across multiple modules, enhancing cross-platform compatibility, particularly for Windows. New methods for checking absolute paths and normalizing paths are added to file system interfaces and implementations. Several internal methods now use these utilities to ensure consistent and correct path processing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FileManager
participant FileSystem
User->>FileManager: getAbsoluteLocation(filename)
FileManager->>FileSystem: isAbsolute(filename)
alt not absolute
FileManager->>FileSystem: resolve path
end
FileManager->>FileSystem: normalize(path)
FileManager-->>User: normalized absolute path
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sourcing/test/FileManager.spec.ts (1)
76-78: Critical test coverage missing for new path normalization functionality.The TODO comments indicate missing tests for
getRelativeLocationmethod and Windows path scenarios, which are crucial for the path normalization functionality introduced in this PR. Without proper test coverage, there's a risk of regressions in cross-platform path handling.Would you like me to help generate comprehensive test cases for:
- The
getRelativeLocationmethod with various path scenarios- Windows-specific path handling (backslashes, drive letters, UNC paths)
- Edge cases like mixed separators and relative path resolution
These tests are essential to ensure the path normalization works correctly across platforms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/build/src/source/module/LocationRewriter.ts(2 hunks)packages/build/src/source/resource/Reader.ts(1 hunks)packages/build/src/source/segment/Reader.ts(1 hunks)packages/jitar/rollup.definitions.js(1 hunks)packages/plugin-vite/src/index.ts(1 hunks)packages/sourcing/src/files/FileManager.ts(2 hunks)packages/sourcing/src/files/LocalFileSystem.ts(2 hunks)packages/sourcing/src/files/RemoteFileSystem.ts(2 hunks)packages/sourcing/src/files/interfaces/FileSystem.ts(2 hunks)packages/sourcing/src/modules/LocalModuleLocator.ts(1 hunks)packages/sourcing/test/FileManager.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: basmasking
PR: MaskingTechnology/jitar#649
File: CHANGELOG.md:6-7
Timestamp: 2025-06-02T10:44:10.408Z
Learning: In CHANGELOG.md for the jitar project, the format `[[PR_NUMBER](link)] description (#ISSUE_NUMBER) by author` is intentionally used where the first number is the pull request ID and the second number in parentheses is the issue ID that the PR addresses. These numbers are often different and this is not an error but a deliberate formatting convention used consistently throughout the changelog.
🧬 Code Graph Analysis (3)
packages/plugin-vite/src/index.ts (1)
packages/init/src/InitManager.ts (1)
rootPath(54-57)
packages/sourcing/src/files/interfaces/FileSystem.ts (3)
packages/sourcing/src/files/FileManager.ts (1)
location(133-142)packages/sourcing/src/files/errors/InvalidLocation.ts (1)
location(13-13)packages/sourcing/src/files/models/File.ts (1)
location(15-15)
packages/sourcing/src/files/RemoteFileSystem.ts (3)
packages/sourcing/src/files/FileManager.ts (1)
location(133-142)packages/sourcing/src/files/errors/InvalidLocation.ts (1)
location(13-13)packages/sourcing/src/files/models/File.ts (1)
location(15-15)
🔇 Additional comments (18)
packages/jitar/rollup.definitions.js (1)
9-9: Good addition of 'path' module as external dependency.Adding the Node.js 'path' module to SERVER_EXTERNALS is necessary since the codebase now uses it for path normalization. This ensures the native module isn't bundled and remains available at runtime.
packages/build/src/source/resource/Reader.ts (1)
56-60: Excellent implementation of path normalization.The addition of path normalization before processing ensures consistent behavior across platforms. The normalized filename is correctly used for both directory checking and extension processing, maintaining the original logic flow while adding cross-platform compatibility.
packages/plugin-vite/src/index.ts (1)
82-86: Comprehensive path normalization implementation.Applying
normalizePathto all constructed paths (rootPath,sourcePath,targetPath,outputPath, andjitarPath) ensures consistent cross-platform path handling throughout the Vite plugin. Using Vite's built-innormalizePathfunction is the correct approach for plugin compatibility.packages/sourcing/src/files/interfaces/FileSystem.ts (2)
12-12: All FileSystem implementations includeisAbsoluteBoth
LocalFileSystem.tsandRemoteFileSystem.tsdefine the newisAbsolute(location: string): booleanmethod, so no further changes are needed:
- packages/sourcing/src/files/LocalFileSystem.ts:
isAbsolute(location: string): boolean { return path.isAbsolute(location); }- packages/sourcing/src/files/RemoteFileSystem.ts:
isAbsolute(location: string): boolean { return location.startsWith('/'); }
24-24: All FileSystem implementations include normalizeBoth
LocalFileSystem.tsandRemoteFileSystem.tsnow define thenormalize(location: string): stringmethod as required. No further action is needed.packages/sourcing/src/modules/LocalModuleLocator.ts (1)
20-24: LGTM! Good URI formation pattern.The change to store the resolved location before returning it with the
file://prefix follows good practices and aligns with the path normalization strategy across the codebase.packages/build/src/source/segment/Reader.ts (1)
129-133: LGTM! Proper normalization placement.The path normalization is correctly applied before directory checks and filename adjustments, ensuring consistent path handling across platforms while maintaining the existing logic flow.
packages/sourcing/src/files/LocalFileSystem.ts (2)
26-29: LGTM! Correct use of Node.js path utilities.The
isAbsolutemethod properly delegates to Node.js'spath.isAbsolute, ensuring platform-appropriate absolute path detection.
80-83: Confirmed:normalizeIs Only Applied to File PathsI searched all
fileSystem.normalize()usages inpackages/sourcing/src/files/FileManager.tsand confirmed they’re exclusively applied to path values (locations, patterns, filenames). There are no instances of it being used on arbitrary content that might legitimately contain backslashes.No changes needed.
packages/sourcing/src/files/RemoteFileSystem.ts (2)
34-37: LGTM! Simple but functional implementation.The
isAbsolutemethod uses a straightforward check for leading slash, which is appropriate for remote file systems. The minimal implementation aligns with the class's role as a placeholder.
64-67: LGTM! Conservative approach for remote systems.The
normalizemethod returning the location unchanged is a safe approach for remote file systems where path modification might not be appropriate or predictable.packages/build/src/source/module/LocationRewriter.ts (2)
48-55: LGTM! Consistent normalization in import rewriting.The path normalization is properly applied before application module checks and path rewriting, ensuring consistent behavior across different input path formats.
70-77: LGTM! Consistent pattern maintained for exports.The export rewriting follows the same normalization pattern as imports, maintaining consistency across the module and ensuring proper path handling for both import and export statements.
packages/sourcing/src/files/FileManager.ts (5)
20-24: LGTM! Constructor normalization ensures consistent state.The constructor now properly normalizes both the location and root location at initialization, which establishes a consistent baseline for all subsequent path operations. This is a solid approach for the Windows compatibility goals.
32-38: LGTM! Improved absolute path handling with proper normalization.The changes enhance cross-platform compatibility by:
- Using the new
isAbsolute()method for proper absolute path detection- Normalizing the absolute path before validation to ensure consistent format
- Maintaining security by validating the normalized path
This approach should handle Windows drive letters and UNC paths correctly.
43-46: LGTM! Consistent path normalization for relative paths.The normalization of relative paths maintains consistency with the overall approach and ensures that relative path operations work correctly across platforms.
48-51: LGTM! Useful public API for external path normalization.The new
normalizeLocationmethod provides a clean public interface for external consumers to normalize paths using the same logic as the FileManager internally. This is particularly valuable for the build system components mentioned in the AI summary.
125-131: LGTM! Comprehensive normalization in filter operations.The normalization of both the input pattern and output filenames ensures that filtering operations work consistently across platforms. The mapping of results through normalization maintains the expected format for all returned paths.
|
2 participants
Fixes #662
Changes proposed in this pull request:
Our internal model remains the posix model, and every input that does not comply with that is translated into it for easier maintenance.
@MaskingTechnology/jitar
Summary by CodeRabbit
New Features
Bug Fixes
Chores