Skip to content
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

Normalize slashes for windows paths #285

Merged
merged 3 commits into from
Mar 21, 2017

Conversation

Jontem
Copy link
Contributor

@Jontem Jontem commented Mar 7, 2017

This PR solves the problem discussed in #283 and should also solve #216. I've only tested on a minimal repro on windows. There already was a function named slash that was exactly implemented as normalizeSlashes that typecript exports. So i just used that one.

@coveralls
Copy link

coveralls commented Mar 7, 2017

Coverage Status

Coverage decreased (-7.9%) to 78.794% when pulling d60ae29 on Jontem:normalize-paths into cf60fd5 on TypeStrong:master.

src/index.ts Outdated
@@ -574,6 +574,7 @@ export function fileExists (fileName: string): boolean {
* Get directories within a directory.
*/
export function getDirectories (path: string): string[] {
console.log(`getDirectories: ${path}`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove the console.log from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Fixed :)

src/index.ts Outdated
@@ -445,14 +445,14 @@ function readConfig (compilerOptions: any, project: string | boolean | undefined
delete result.config.compilerOptions.outFile
delete result.config.compilerOptions.declarationDir

const basePath = result.path ? dirname(result.path) : cwd
const basePath = result.path ? normalizeSlashes(dirname(result.path)) : cwd
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you should be normalizing the slashes on both sides, not just the left side of the ternary.

Copy link
Contributor Author

@Jontem Jontem Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Interesting note is that without a tsconfig.json this problem did not exists for me. But as soon as i added a tsconfig.json i got the error. It seems that typescript handles basePath differently as cwd also uses the windows path separator. Any how the less confusing way would be to treat them the same as you said.

src/index.ts Outdated
@@ -445,14 +445,15 @@ function readConfig (compilerOptions: any, project: string | boolean | undefined
delete result.config.compilerOptions.outFile
delete result.config.compilerOptions.declarationDir

const basePath = result.path ? dirname(result.path) : cwd
const normalizedConfigPath = result.path && normalizeSlashes(result.path)
Copy link
Contributor Author

@Jontem Jontem Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized we need to handle when no tsconfig.json is found. Can't do replace on undefined. So fixed that to.

@coveralls
Copy link

coveralls commented Mar 8, 2017

Coverage Status

Coverage decreased (-7.4%) to 79.296% when pulling 16a7d63 on Jontem:normalize-paths into cf60fd5 on TypeStrong:master.

@massimocode
Copy link

@Jontem thanks for picking this up! 👍

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.008%) to 83.696% when pulling 0f14d2b on Jontem:normalize-paths into cf60fd5 on TypeStrong:master.

@blakeembrey blakeembrey merged commit 721641e into TypeStrong:master Mar 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants