-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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: use path.normalize
to convert POSIX paths to OS-specific paths
#5327
Conversation
🦋 Changeset detectedLatest commit: 1050325 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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 |
Hi @haines, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
864bb72
to
ae46139
Compare
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
8cd73bd
to
9b28e1f
Compare
Hey @haines, could you target the |
9b28e1f
to
1050325
Compare
@machour done! Could you please re-approve the workflow run? |
@@ -28,7 +28,7 @@ export function flatRoutes( | |||
// fast-glob will return posix paths even on windows | |||
// convert posix to os specific paths | |||
let routePathsForOS = routePaths.map((routePath) => { | |||
return path.join(...routePath.split(path.posix.sep)); | |||
return path.normalize(routePath); |
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.
TIL 👍
As far as I see, this PR is modifying a test already passing. |
@machour the existing test just mirrors the production code, so the test has the same bug and it needs to be modified. The problem is that the test exercises the internal function I could rewrite the test to exercise |
Superseded by #5228 |
🤖 Hello there, We just published version Thanks! |
Closes: #5322
(from #5324): The build currently breaks when loading routes
Note it is trying to load from
outes
notroutes
!The bug was introduced in #5266. By splitting and rejoining the paths, it drops the leading
/
on Unix-like systems, converting/Users/me/...
toUsers/me/...
. HoweverappDirectory
still has a leading/
so removing directory prefixes withslice
drops one too many characters.This PR resolves the issue by using
path.normalize
to covert/
to\
on Windows, rather than splitting and rejoining the paths.Testing Strategy:
This test covers this code (sort of... the test mirrors the production code so does not expose the bug):
remix/packages/remix-dev/__tests__/flat-routes-test.ts
Line 141 in ebfa645