fix: Support generation on Windows machines #402
Merged
Conversation
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
🦋 Changeset detectedLatest commit: e7540d4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request introduces comprehensive Windows platform compatibility improvements across multiple packages. The changes focus on enhancing path handling, module importing, and file resolution mechanisms to ensure consistent behavior across different operating systems. Key modifications include using Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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 (
|
There was a problem hiding this comment.
Copilot reviewed 5 out of 11 changed files in this pull request and generated 2 comments.
Files not reviewed (6)
- packages/core-generators/src/generators/node/prettier/index.ts: Evaluated as low risk
- packages/project-builder-lib/src/plugin-tools/plugin-loader.ts: Evaluated as low risk
- packages/project-builder-server/src/plugins/node-plugin-store.ts: Evaluated as low risk
- plugins/baseplate-plugin-storage/src/generators/fastify/storage-module/index.ts: Evaluated as low risk
- packages/project-builder-test/src/runner/discover-tests.ts: Evaluated as low risk
- packages/core-generators/src/writers/typescript/imports.ts: Evaluated as low risk
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core-generators/src/utils/path.ts (1)
3-6: Update JSDoc to match the new parameter name.The JSDoc still references the old parameter name
pathwhile the function now usesfilePath./** * Builds out the full path so it can be used for imports/writing to file - * @param path Path to the file with file extension + * @param filePath Path to the file with file extension */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (11)
.changeset/fair-eels-kiss.md(1 hunks)packages/core-generators/src/generators/node/prettier/index.ts(3 hunks)packages/core-generators/src/utils/path.ts(1 hunks)packages/core-generators/src/writers/typescript/imports.ts(1 hunks)packages/project-builder-lib/src/plugin-tools/plugin-loader.ts(1 hunks)packages/project-builder-server/src/plugins/node-plugin-store.ts(2 hunks)packages/project-builder-test/src/runner/discover-tests.ts(2 hunks)packages/sync/src/core/generator-output.ts(1 hunks)packages/sync/src/core/loader.ts(1 hunks)packages/sync/src/utils/require.ts(1 hunks)plugins/baseplate-plugin-storage/src/generators/fastify/storage-module/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- .changeset/fair-eels-kiss.md
- packages/project-builder-server/src/plugins/node-plugin-store.ts
- packages/project-builder-lib/src/plugin-tools/plugin-loader.ts
- packages/sync/src/utils/require.ts
- packages/core-generators/src/writers/typescript/imports.ts
- packages/project-builder-test/src/runner/discover-tests.ts
- plugins/baseplate-plugin-storage/src/generators/fastify/storage-module/index.ts
- packages/sync/src/core/generator-output.ts
- packages/core-generators/src/generators/node/prettier/index.ts
- packages/sync/src/core/loader.ts
🔇 Additional comments (2)
packages/core-generators/src/utils/path.ts (2)
7-13: LGTM! Windows path normalization looks good.
The implementation correctly handles Windows path separators by normalizing them to POSIX style, ensuring consistent import paths across different operating systems.
1-19: Verify consistent path handling across the codebase.
Let's ensure these path utilities are used consistently throughout the codebase to maintain Windows compatibility.
✅ Verification successful
Path handling utilities are correctly implemented and used consistently
The codebase demonstrates proper Windows compatibility through:
- Consistent usage of
makeImportAndFilePathfor import paths across React and Fastify generators - Proper path separator normalization using
path.septopath.posix.sepin core utilities - Limited and appropriate usage of
unixPathJoinfor specific cases requiring POSIX paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential path handling issues in the codebase
# Find direct path.join usage that might need unixPathJoin
echo "Checking for path.join usage that might need unixPathJoin:"
rg "path\.join\(" --type ts
# Find potential Windows path separator usage
echo -e "\nChecking for hardcoded Windows path separators:"
rg "\\\\+" --type ts
# Find potential path string concatenation that might need unixPathJoin
echo -e "\nChecking for path string concatenation:"
rg "['\`\"]\/.*\+.*\/['\`\"]" --type ts
Length of output: 35398
Script:
#!/bin/bash
# Let's check for direct path handling in the codebase to verify Windows compatibility
# Search for direct path string concatenation patterns
echo "Checking for path string concatenation patterns:"
rg -A 2 "['\"`]/.*?\+" --type ts
# Search for path.sep usage to ensure Windows compatibility
echo -e "\nChecking path.sep usage:"
rg "path\.sep" --type ts
# Search for makeImportAndFilePath usage
echo -e "\nChecking makeImportAndFilePath usage:"
rg "makeImportAndFilePath" --type ts
# Search for unixPathJoin usage
echo -e "\nChecking unixPathJoin usage:"
rg "unixPathJoin" --type ts
Length of output: 223
Script:
#!/bin/bash
# Let's check for direct path handling in the codebase to verify Windows compatibility
# Search for direct path string concatenation patterns (fixed syntax)
echo "Checking for path string concatenation patterns:"
rg "['\"]\/" --type ts -A 2
# Search for path.sep usage to ensure Windows compatibility
echo -e "\nChecking path.sep usage:"
rg "path\.sep" --type ts
# Search for makeImportAndFilePath usage
echo -e "\nChecking makeImportAndFilePath usage:"
rg "makeImportAndFilePath" --type ts
# Search for unixPathJoin usage
echo -e "\nChecking unixPathJoin usage:"
rg "unixPathJoin" --type ts
Length of output: 15071
Comment on lines
15
to
19
| export function unixPathJoin(...paths: string[]): string { | ||
| return paths | ||
| .join(path.posix.sep) | ||
| .replaceAll(`${path.posix.sep}${path.posix.sep}`, path.posix.sep); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc documentation and improve path handling.
The function needs documentation and could benefit from more robust path handling.
- Add JSDoc documentation:
+/**
+ * Joins path segments using POSIX separators, ensuring consistent behavior across platforms
+ * @param paths - Array of path segments to join
+ * @returns Joined path using POSIX separators
+ */
export function unixPathJoin(...paths: string[]): string {- Consider using
path.posix.normalizefor more robust path handling:
export function unixPathJoin(...paths: string[]): string {
- return paths
- .join(path.posix.sep)
- .replaceAll(`${path.posix.sep}${path.posix.sep}`, path.posix.sep);
+ return path.posix.normalize(
+ paths
+ .filter(Boolean)
+ .join(path.posix.sep)
+ );
}This improvement:
- Handles empty or undefined path segments
- Normalizes relative path segments (../. or ./)
- Consolidates consecutive separators
- Maintains correct behavior for absolute paths
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function unixPathJoin(...paths: string[]): string { | |
| return paths | |
| .join(path.posix.sep) | |
| .replaceAll(`${path.posix.sep}${path.posix.sep}`, path.posix.sep); | |
| } | |
| /** | |
| * Joins path segments using POSIX separators, ensuring consistent behavior across platforms | |
| * @param paths - Array of path segments to join | |
| * @returns Joined path using POSIX separators | |
| */ | |
| export function unixPathJoin(...paths: string[]): string { | |
| return path.posix.normalize( | |
| paths | |
| .filter(Boolean) | |
| .join(path.posix.sep) | |
| ); | |
| } |
kingston
deleted the
kingston/eng-511-enable-generation-on-windows-machines
branch
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Allow for \ path separators in generations
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor