-
Notifications
You must be signed in to change notification settings - Fork 73
Represent URIs as WHATWG URLs #203
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #203 +/- ##
==========================================
- Coverage 55.53% 54.53% -1.01%
==========================================
Files 16 15 -1
Lines 1977 1951 -26
Branches 313 305 -8
==========================================
- Hits 1098 1064 -34
- Misses 760 773 +13
+ Partials 119 114 -5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Windows it doesn't work from VS code:
ERROR Handler for textDocument/hover failed: { Error: ENOENT: no such file or di
rectory, open 'c:\projects\javascript-typescript-langserver\c:\projects\javascri
pt-typescript-langserver\node_modules\@reactivex\rxjs\dist\cjs\package.json'
errno: -4058,
code: 'ENOENT',
syscall: 'open',
path: 'c:\\projects\\javascript-typescript-langserver\\c:\\projects\\javascrip
t-typescript-langserver\\node_modules\\@reactivex\\rxjs\\dist\\cjs\\package.json
' }
package.json
Outdated
@@ -61,7 +62,7 @@ | |||
"@types/lodash": "4.14.63", | |||
"@types/mocha": "2.2.40", | |||
"@types/mz": "0.0.31", | |||
"@types/node": "7.0.14", | |||
"@types/node": "6.0.70", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why, mister Anderson?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we the minimum Node version we can rely on is Node 6 (LTS, used by Electron). If we used Node 7, we would be able to use require('url').URL
instead of the polyfill
protected resolveUriToPath(uri: string): string { | ||
return toUnixPath(path.resolve(this.rootPath, uri2path(uri))); | ||
protected resolveUriToPath(uri: URL): string { | ||
return uri2path(uri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove resolveUriToPath
method then and replace it with direct uri2path
invocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried it (if you look at commit log) but it is useful for overriding it. Otherwise the globbing etc. needs to be completely reimplemented.
src/fs.ts
Outdated
}); | ||
return iterate(files).map(file => baseUri + file.split('/').map(encodeURIComponent).join('/')); | ||
return iterate(files).map(file => new URL(file.split('/').map(encodeURIComponent).join('/'), base.href + '/')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it handle special characters properly? I see the following example in https://url.spec.whatwg.org/#url:
var input = "https://example.org/💩",
url = new URL(input)
url.pathname // "/%F0%9F%92%A9"
There input is non-encoded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting to leave out the encodeURIComponent
call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I'd try to remove it and see if it works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, encoding is done automatically for the path, but not all characters. Specifically, @
characters are not encoded. I don't think this is a problem.
uris(): IterableIterator<string> { | ||
return this.files.keys(); | ||
uris(): IterableIterator<URL> { | ||
return iterate(this.files.keys()).map(uri => new URL(uri)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java does it better map(URL::new)
😀
src/project-manager.ts
Outdated
// TypeScript works with file paths, not URIs | ||
const filePath = parts.pathname.split('/').map(decodeURIComponent).join('/'); | ||
const filePath = uri.pathname.split('/').map(decodeURIComponent).join('/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uri2path
for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want Windows paths here? (Probably we do)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that having unified POSIX paths would be better for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would want a Windows user to see C:\Users\felix\myfile.ts
as a containerName
for a class though, not /C:/Users/felix/myfile.ts
src/project-manager.ts
Outdated
@@ -370,16 +366,16 @@ export class ProjectManager implements Disposable { | |||
); | |||
}) | |||
// Use same scheme, slashes, host for referenced URI as input file | |||
.map(filePath => url.format({ ...parts, pathname: filePath.split(/[\\\/]/).map(encodeURIComponent).join('/'), search: undefined, hash: undefined })) | |||
.map(filePath => new URL(filePath.split(/[\\\/]/).map(encodeURIComponent).join('/'), uri.href)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path2uri
for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
src/util.ts
Outdated
} | ||
p = toUnixPath(p).split('/').map(encodeURIComponent).join('/'); | ||
return ret + p; | ||
const pathname = parts.join(isWindowsUri ? '\\' : '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't isWindowsUri
check redundant here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? We need to decide which separator to use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave POSIX style
src/util.ts
Outdated
// %-decode parts | ||
let filePath = decodeURIComponent(uri.pathname); | ||
// Strip the leading slash on Windows | ||
const isWindowsUri = /^\/[a-z]:\//.test(filePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make case-insensitive
.map(textDocument => textDocument.uri); | ||
async getWorkspaceFiles(base?: URL, childOf = new Span()): Promise<Iterable<URL>> { | ||
return iterate(await this.client.workspaceXfiles({ base: base && base.href }, childOf)) | ||
.map(textDocument => new URL(textDocument.uri)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path2uri
for converting file paths, new URL
for converting a string-URL to a URL
object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
298fd28
to
a26f80d
Compare
a26f80d
to
55f8769
Compare
55f8769
to
d8f9e44
Compare
Codecov Report
@@ Coverage Diff @@
## master #203 +/- ##
==========================================
- Coverage 60.2% 54.57% -5.64%
==========================================
Files 14 15 +1
Lines 2151 1946 -205
Branches 351 305 -46
==========================================
- Hits 1295 1062 -233
- Misses 706 772 +66
+ Partials 150 112 -38
Continue to review full report at Codecov.
|
This uses the WHATWG
URL
objects everywhere a URI was previously saved, passed or returned as a string and replaces usages ofurl.parse()
andurl.format()
. This makes itAlso reimplements
path2uri
anduri2path
.rootUri
is now passed into more places to be used as thepath2uri
root.@alexsaveliev any Windows incompatibilities you see?
Closes #125