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

Fix #595 #596

Merged
merged 2 commits into from
May 24, 2018
Merged

Fix #595 #596

merged 2 commits into from
May 24, 2018

Conversation

ljani
Copy link
Contributor

@ljani ljani commented May 23, 2018

The cwd should be normalized, see:
https://github.com/Microsoft/TypeScript/blob/16d7f4c1033cf1852a2904845b0568f932ed4f21/src/compiler/tsc.ts#L104

normalizePath changes eg. C:\my\awesome\project to C:/my/awesome/project on Windows.

The test-fail-2 in #595 seems to be related to #409 though.

@ljani
Copy link
Contributor Author

ljani commented May 23, 2018

Any idea how to call normalizePath? It is exported here and it does exist in the javascript variable ts, but the type definition seems to be missing.

Apparently reading the TODO comment, it's supposed to be replaced with resolvePath.

EDIT: based on my testing ts.sys.resolvePath is not the same as ts.normalizePath though? The latter will change \ to /, but the former won't.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.224% when pulling a26807d on ljani:master into df41bdb on TypeStrong:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.224% when pulling a26807d on ljani:master into df41bdb on TypeStrong:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.224% when pulling a26807d on ljani:master into df41bdb on TypeStrong:master.

@coveralls
Copy link

coveralls commented May 23, 2018

Coverage Status

Coverage remained the same at 84.224% when pulling c4ca001 on ljani:master into df41bdb on TypeStrong:master.

@blakeembrey
Copy link
Member

Would this also be a problem with using resolve? Should that also be wrapped? The only way I know to use it is to just duplicate the function (I thought it already was somewhere, but that must have been the old code). It appears I’m wrong about searching for the project, I didn’t think I supported t but maybe someone did a PR, or maybe I just misunderstood that function.

@ljani
Copy link
Contributor Author

ljani commented May 23, 2018

@blakeembrey I'm not following 100% what do you mean with resolve. If you mean all current calls to resolve, I have no idea. If you mean the node API path.resolve or TypeScripts' ts.sys.resolve, they both return (absolute) paths with backslashes on Windows. It won't fix the issue as TypeScript's findConfigFile function expects forward slashes on Windows as well.

I'm not familiar with typescript's API at all and thus I was hoping you (or anyone) could help me.

And yes, TypeScript's findConfigFile seems to be searching parent directories, so it seems you misunderstood the function :)

@blakeembrey
Copy link
Member

There's a function that should do what you want already:

ts-node/src/index.ts

Lines 133 to 135 in df41bdb

export function normalizeSlashes (value: string): string {
return value.replace(/\\/g, '/')
}
.

What I meant is that on the same line, I use resolve(cwd, project) - this seems like it'd also break in Windows since it would output backslashes and can also use the normalize function on it.

@ljani
Copy link
Contributor Author

ljani commented May 23, 2018

There's a function that should do what you want already

Great, thanks!

What I meant is that on the same line, I use resolve(cwd, project) - this seems like it'd also break in Windows since it would output backslashes and can also use the normalize function on it.

That's true, nice catch. Let me fix the PR.

@ljani
Copy link
Contributor Author

ljani commented May 23, 2018

The commit 86176de seems to also fix the test-fail-2 case as well. This is great!

EDIT: Fixed the commit message

The cwd and configFileName should be normalized, see:
https://github.com/Microsoft/TypeScript/blob/16d7f4c1033cf1852a2904845b0568f932ed4f21/src/compiler/tsc.ts#L104

This affects only Windows where backslash is the path separator.
@ljani ljani changed the title Fix test-fail-1 in #595 Fix #595 May 23, 2018
@blakeembrey blakeembrey merged commit 45664e7 into TypeStrong:master May 24, 2018
@ljani
Copy link
Contributor Author

ljani commented May 24, 2018

@blakeembrey Your commit (c4ca001) does not fix the test-fail-2 as configFileName is passed to parseJsonConfigFileContent as well.

@blakeembrey
Copy link
Member

@ljani That's why I typically avoid code that mutates a variables, it wasn't clear it was to fix multiple spots. It would be clearer added around the resolve().

@blakeembrey
Copy link
Member

Fixed with 33aca82.

@ljani
Copy link
Contributor Author

ljani commented May 24, 2018

Thanks. Yours is much better!

Yeah, I was avoiding to adding it around resolve(), because I do not know if it can return null or undefined in some circumstances, but it shouldn't according to the typings. JS-mode kicked in for a bit.

@blakeembrey
Copy link
Member

@ljani Haha, thanks. Using TypeScript definitely has it's perks there 😄

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.

None yet

3 participants