-
Notifications
You must be signed in to change notification settings - Fork 310
fix(cli): normalize Windows paths for module identifiers and add test… #148
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
base: main
Are you sure you want to change the base?
Conversation
@nipunn1313 , can you look into it. |
Been a little busy - will try to get to it with some time. Thanks for the contribution. |
let noDrive = posixRelPath.replace(/^[A-Za-z]:\\|^[A-Za-z]:\//, ""); | ||
// Convert all backslashes to forward slashes | ||
return noDrive.replace(/\\/g, "/"); | ||
} |
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.
I suspect we're going to want to use standard libraries rather than doing our own custom string crunching like this.
Windows paths are incredibly complex and there are a lot of ways they can be represented - sticking to standard libraries rather than writing custom will be worthwhile.
https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats
Would also appreciate if you could look into where the module path is used and why it's sent up to the server. That may help us understand what the right solution is. In general, having OS-specific paths sent to the server is probably not the right architecture since the same convex backend can be developed with from multiple OSes.
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.
Thanks @nipunn1313 , i will take a look at standard libraries for that, also will check usage & why it is sent up to server. Thanks for the input
Hey @nipunn1313, thanks for the feedback! You're absolutely right about using standard libraries and understanding the architecture better. I dug into how module paths actually work and found some interesting things: The real issue: Module paths aren't file paths at all - they're logical identifiers like Looking at the server code in
So when someone on Windows has New approach using standard lib: export function normalizeModulePath(outputPath: string, baseDir: string = "out"): string {
// Normalize separators first
const normalizedOutput = outputPath.replace(/\\/g, '/');
const normalizedBase = baseDir.replace(/\\/g, '/');
const relativePath = path.posix.relative(normalizedBase, normalizedOutput);
// For absolute paths, find where the base dir appears and extract what comes after
if (relativePath.startsWith('../')) {
const outputParts = normalizedOutput.split('/');
const baseParts = normalizedBase.split('/');
const baseIndex = outputParts.findIndex((part, index) => {
return baseParts.every((basePart, bIndex) =>
outputParts[index + bIndex] === basePart
);
});
if (baseIndex >= 0) {
return outputParts.slice(baseIndex + baseParts.length).join('/');
}
return outputParts[outputParts.length - 1];
}
return relativePath;
} This way we're using Tests are all passing now - Windows paths like |
@nipunn1313 , Had a chance to look into it ? |
Hi @kmr-rohit - not yet. Lot going on over at Convex - it's in my queue. |
Totally understandable, @nipunn1313 — with everything going on at Convex and being early adopters of PlanetScale’s Postgres, it’s all shaping up as it’s meant to be. |
… (#140)
fix(cli): issue #140 - normalize Windows paths for module identifiers and added tests for windows paths.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.