Represent URIs as WHATWG URLs #203

felixfbecker · 2017-04-27T21:35:17Z

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 · 2017-04-27T21:35:21Z

Codecov Report

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

@@            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.

alexsaveliev

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
' }

alexsaveliev · 2017-04-28T07:36:16Z

package.json

@@ -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",


Why, mister Anderson?

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

alexsaveliev · 2017-04-28T07:37:31Z

src/fs.ts

-	protected resolveUriToPath(uri: string): string {
-		return toUnixPath(path.resolve(this.rootPath, uri2path(uri)));
+	protected resolveUriToPath(uri: URL): string {
+		return uri2path(uri);


I'd remove resolveUriToPath method then and replace it with direct uri2path invocation

Tried it (if you look at commit log) but it is useful for overriding it. Otherwise the globbing etc. needs to be completely reimplemented.

alexsaveliev · 2017-04-28T07:41:47Z

src/fs.ts

 		});
-		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 + '/'));


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

Are you suggesting to leave out the encodeURIComponent call?

yes, I'd try to remove it and see if it works

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.

alexsaveliev · 2017-04-28T07:45:56Z

src/memfs.ts

-	uris(): IterableIterator<string> {
-		return this.files.keys();
+	uris(): IterableIterator<URL> {
+		return iterate(this.files.keys()).map(uri => new URL(uri));


Java does it better map(URL::new) 😀

alexsaveliev · 2017-04-28T07:48:07Z

src/project-manager.ts

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


uri2path for consistency?

Do we want Windows paths here? (Probably we do)

I think that having unified POSIX paths would be better for consistency

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

alexsaveliev · 2017-04-28T07:48:54Z

src/project-manager.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))


path2uri for consistency?

Makes sense

alexsaveliev · 2017-04-28T07:55:23Z

src/util.ts

 	}
-	p = toUnixPath(p).split('/').map(encodeURIComponent).join('/');
-	return ret + p;
+	const pathname = parts.join(isWindowsUri ? '\\' : '/');


Isn't isWindowsUri check redundant here?

What do you mean? We need to decide which separator to use

I'd leave POSIX style

alexsaveliev · 2017-04-28T07:55:54Z

src/util.ts

+	// %-decode parts
+	let filePath = decodeURIComponent(uri.pathname);
+	// Strip the leading slash on Windows
+	const isWindowsUri = /^\/[a-z]:\//.test(filePath);


please make case-insensitive

alexsaveliev · 2017-04-28T08:02:22Z

src/fs.ts

-			.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));


Could we somehow made it consistent - either new URI across the code or path2uri?

path2uri for converting file paths, new URL for converting a string-URL to a URL object

codecov-io · 2018-07-04T07:31:26Z

Codecov Report

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

@@            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.

felixfbecker added the enhancement label Apr 27, 2017

felixfbecker assigned alexsaveliev Apr 27, 2017

felixfbecker requested a review from alexsaveliev April 27, 2017 21:35

alexsaveliev suggested changes Apr 28, 2017

View reviewed changes

felixfbecker mentioned this pull request Apr 29, 2017

Test different root URIs #206

Merged

felixfbecker force-pushed the whatwg-url branch from 298fd28 to a26f80d Compare April 29, 2017 15:46

Use WHATWG URL instead of url.parse()

4dcdcab

felixfbecker force-pushed the whatwg-url branch from a26f80d to 55f8769 Compare April 29, 2017 17:21

felixfbecker added 10 commits April 29, 2017 23:46

Use URL for most functions

574b313

Pass rootUri

cea66d7

Reintroduce resolveUriToPath()

cb22fdd

Make ensureReferencedFiles() return URLs

6845455

Let URL handle encoding

63d9cd9

Use path2uri()

91a7854

Compare drive letters case-insensitively

fd2b212

Use path2uri in fs

3026760

use path.sep

87af881

Add more tests for special chars

d8f9e44

felixfbecker force-pushed the whatwg-url branch from 55f8769 to d8f9e44 Compare April 30, 2017 14:47

felixfbecker unassigned alexsaveliev May 8, 2017

Represent URIs as WHATWG URLs #203

Are you sure you want to change the base?

Represent URIs as WHATWG URLs #203

Uh oh!

Conversation

felixfbecker commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexsaveliev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixfbecker Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jul 4, 2018

Codecov Report

Uh oh!

Uh oh!

felixfbecker commented Apr 27, 2017 •

edited

Loading

codecov bot commented Apr 27, 2017 •

edited

Loading

felixfbecker Apr 28, 2017 •

edited

Loading