Normalize appRootPath to prevent issues on Windows #3028
Conversation
| @@ -1,8 +1,11 @@ | |||
| import { fileExists } from './fileutils'; | |||
| import * as path from 'path'; | |||
|
|
|||
| // TODO: vsavkin normalize the path | |||
| export const appRootPath = pathInner(__dirname); | |||
| function normalizeBackslashes(dir) { | |||
There was a problem hiding this comment.
While looking at this, I think we should stay path delimiter separator agnostic instead of forcing windows into using forward slashes. Otherwise we could run into other issues in unknown places where app-root is being used. Apart from this it's a good start for a discussion. I'm excited to see the CI/CD results if this fixes it or if implications of this change are correctly revealed by the CI/CD.
Let's wait for some maintainer comments. Thank you for crafting this PR 👍
Reference:
https://nodejs.org/api/path.html#path_path_delimiter
https://nodejs.org/api/path.html#path_path_sep
There was a problem hiding this comment.
I guess you meant https://nodejs.org/api/path.html#path_path_sep not delimiter. You may be right, let's wait for maintainers feedback.
There was a problem hiding this comment.
Oh yes, I just searched for a path example and cited the wrong section. I wanted to mention the path separator of course. Thanks for pointing out 👍
|
FYI. The CI failed for a formating issue and prevented to run any unit test tests from what I see. |
|
I just found out about prior art by @rhutchison, opened 21 days ago, marked ready 6 days ago I see that the requested change inside the app-root export was not addressed for some good reasons ("too risky"). He points out that the impact of changing the appRoot globally for Nx is quite big and changing it in only this place could fix the issue with limited impact. That's a good point. The fix is the same and it's also missing an agnostic approach using the OS path separator. Would be good to get some guidance here. Can we trust the Nx Test Suite to just try it with your proposal or work on a OS agnostic solution? At least you provided a solution matching the changes requested by @FrozenPandaz in the other PR.
|
|
I merged #2941 which solves the same issue so I'm going to close this PR. Thank you for your contribution! |
|
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
workspace/utils/app-root - appRootPath constant was un-normalized until now.
This lead to issues with dep-graph on Windows systems.
Fixes path-based detection #2955