Skip to content

Use dedicated type to store paths #5462

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

Merged
merged 5 commits into from
Oct 30, 2015

Conversation

vladima
Copy link
Contributor

@vladima vladima commented Oct 29, 2015

This PR introduces Path type to store normalized, canonicalized absolute paths. Currently we use plain strings and we have to conservatively normalize them every time we want to use them as key in map. With Path we can use toPath function to convert string to path and then rely on type system to prevent incorrect usages.

Also this PR partially reverts effect from #5275 - now we don't convert resolved module name to relative when we retrieve source file from the host. As a consequence of this now source files in program can have mixed styled file names (absolute and relative). In order to unpleasant user experience when file names in error messages have different representation we convert all file names to relative before printing them

@@ -2183,6 +2182,12 @@ namespace ts {
return result;
}

export function toRelativePath(absoluteOrRelativePath: string, basePath: string, getCanonicalFileName: (path: string) => string): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. other helpers have a verb to begin them, like convert, get.. etc..

@mhegazy
Copy link
Contributor

mhegazy commented Oct 30, 2015

👍

vladima added a commit that referenced this pull request Oct 30, 2015
…ingImports

Use dedicated type to store paths
@vladima vladima merged commit 43b17c1 into master Oct 30, 2015
@vladima vladima deleted the useResolvedFileNameWhenResolvingImports branch October 30, 2015 17:09
@DanielRosenwasser
Copy link
Member

👏 👏 👏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants