Fix ts config path resolve on Windows #14080
Conversation
|
|
Convly
added
pr: fix
| @@ -16,7 +16,7 @@ const DEFAULT_TS_CONFIG_FILENAME = 'tsconfig.json'; | |||
| * @return {string | undefined} | |||
| */ | |||
| module.exports = (dir, { filename = DEFAULT_TS_CONFIG_FILENAME, ancestorsLookup = false } = {}) => { | |||
| const dirAbsolutePath = path.resolve(dir); | |||
| const dirAbsolutePath = path.resolve(dir).split(path.sep).join(path.posix.sep); | |||
There was a problem hiding this comment.
Maybe I'm missing something, but I'm not sure to understand the need here. Isn't path.resolve supposed to return the path with os path separator anyway?
There was a problem hiding this comment.
It does return it using the platform-specific path segment separator, correct.
This is why we need to manually fix this, as the result will be different depending on the OS we are on:
\on Windows/on POSIX
ts.findConfigFile() on line 20 always returns the path with POSIX style path separators (e.g. "foo/bar").
When we then compare it to the Windows style path that path.resolve() returns using .startsWith() in line 26, we are comparing "foo/bar" with "foo\bar" which will always return false.
This is what my change is doing, it ensures that the path is always using the POSIX style path separators, even on Windows. This is why this only fails on Windows and was most likely overlooked during development of this change.
There was a problem hiding this comment.
This is the correct fix.
I would also add ".split(path.sep).join(path.posix.sep);" to configFilePath just in case it changes in typescript in the future.
path.normalize() might also be needed.
There was a problem hiding this comment.
Thanks for the clarifications.
I'm now just wondering what happens if we try to compare the configFilePath (always posix) to the dirAbsolutePath (sep bound to the OS) on line 26. Shouldn't we do the check on two posix paths instead?
There was a problem hiding this comment.
Basically for configFilePath.startsWith(dirAbsolutePath) with configFilePath as a / separated path and dirAbsolutePath with separators that depends on the OS, we can have mismatches, right?
There was a problem hiding this comment.
The issue you describe is what is solved with the proposed change.
The path seperator of dirAbsolutePath was dependent on the OS of the host before, but by splitting it by the OS sep and joining with the POSIX sep (.split(path.sep).join(path.posix.sep)) we convert it into POSIX format, thus now comparing POSIX with POSIX style separators in line 26.
I didn't want to touch any other logic so this was the only change I made.
There was a problem hiding this comment.
@Convly What's holding us back from getting this merged? I feel like this should be fixed ASAP as it's preventing Windows users from starting any TS Strapi projects.
|
Thank you a lot for your contribution to this PR |
What does it do?
This fixes the issue described in #14079
Why is it needed?
The check for an existing TS config does not work on Windows due to the paths being compared are using different path separators based on the OS in use. Since the POSIX style path will always be different than for example the same path on Windows, the check to see if they are included in one another fails.
How to test it?
As described in #14079: start a TS project on Windows.
The config detection will not work.
Related issue(s)/PR(s)
Related to #14079