Skip to content
This repository was archived by the owner on Oct 16, 2020. It is now read-only.

Represent URIs as WHATWG URLs #203

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Represent URIs as WHATWG URLs #203

wants to merge 11 commits into from

Conversation

felixfbecker
Copy link
Contributor

@felixfbecker felixfbecker commented Apr 27, 2017

This uses the WHATWG URL objects everywhere a URI was previously saved, passed or returned as a string and replaces usages of url.parse() and url.format(). This makes it

  • easier to work with the URIs - access segments without explicitly parsing before
  • more typesafe, it's impossible now to pass a file path to a function taking a URI

Also reimplements path2uri and uri2path.
rootUri is now passed into more places to be used as the path2uri root.

@alexsaveliev any Windows incompatibilities you see?

Closes #125

@codecov
Copy link

codecov bot commented Apr 27, 2017

Codecov Report

Merging #203 into master will decrease coverage by 1%.
The diff coverage is 83.94%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/util.ts 69.1% <100%> (-4.97%) ⬇️
src/fs.ts 81.03% <73.68%> (-1.73%) ⬇️
src/memfs.ts 81.57% <80%> (-0.86%) ⬇️
src/typescript-service.ts 77.03% <80.76%> (+0.45%) ⬆️
src/project-manager.ts 79.4% <87.87%> (+0.2%) ⬆️
src/lang-handler.ts 0% <0%> (-11.91%) ⬇️
src/match-files.ts 65.02% <0%> (-0.5%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ec6e7b...d8f9e44. Read the comment docs.

Copy link
Contributor

@alexsaveliev alexsaveliev left a 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",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why, mister Anderson?

Copy link
Contributor Author

@felixfbecker felixfbecker Apr 28, 2017

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);
Copy link
Contributor

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

Copy link
Contributor Author

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 + '/'));
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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));
Copy link
Contributor

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) 😀

// TypeScript works with file paths, not URIs
const filePath = parts.pathname.split('/').map(decodeURIComponent).join('/');
const filePath = uri.pathname.split('/').map(decodeURIComponent).join('/');
Copy link
Contributor

Choose a reason for hiding this comment

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

uri2path for consistency?

Copy link
Contributor Author

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)

Copy link
Contributor

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

Copy link
Contributor Author

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

@@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

path2uri for consistency?

Copy link
Contributor Author

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 ? '\\' : '/');
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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);
Copy link
Contributor

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));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we somehow made it consistent - either new URI across the code or path2uri?
there can be only one

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@codecov-io
Copy link

Codecov Report

Merging #203 into master will decrease coverage by 5.63%.
The diff coverage is 84.05%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/util.ts 68.06% <100%> (-12.4%) ⬇️
src/fs.ts 81.03% <73.68%> (-7.86%) ⬇️
src/memfs.ts 81.57% <76.47%> (-4.31%) ⬇️
src/typescript-service.ts 76.96% <80.39%> (+4.39%) ⬆️
src/project-manager.ts 79.7% <91.89%> (-3.52%) ⬇️
src/lang-handler.ts 0% <0%> (-15%) ⬇️
src/logging.ts 38.46% <0%> (-10.26%) ⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f425f3...d8f9e44. Read the comment docs.

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

Successfully merging this pull request may close these issues.

3 participants