refactor: replace node url parser

[idleberg]

url.parse has been deprecated a long time ago in favour of the URL class, which was added in NodeJS 6.13 and exposed globally in NodeJS 10:

> const repoPath = url.parse('git+https://github.com/pulsar-edit/terminal.git').pathname
'/pulsar-edit/terminal.git'
> (node:55895) [DEP0169] DeprecationWarning: `url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.

[confused-Techie]

Thanks a ton for contributing!

This PR looks good, and I've confirmed that the properties we get back in each instance is exactly what we would get back from the deprecated method we've been using. So things should be 100% backwards compatible.

Just letting the tests run to confirm this then we can hopefully merge!

[confused-Techie]

Well that's a first, everything but Windows tests are failing.
Makes me somewhat worried it's related to #171 but all tests passed on the PR, so seems strange.

May want to do a quick //silenceOutput() in the init-spec.js file to see what's going on here.

Otherwise I may need to spin up a Linux VM to try and see what's happening.

[idleberg]

I didn't manage to get the build running on macOS, but I remember this was already a painful process in the Atom days. However, I didn't expect any issues so I had faith in committing without running the tests.

[confused-Techie]

@idleberg No worries at all, I'd also assume this is a small enough change to not warrant much concern.

I'll see what I can find out

[confused-Techie]

I've gone ahead and done some testing.

The root of the issue here is that the WHATWG URL class has one key difference from the NodeJS module. If you give it an invalid URL it'll throw an exception.

And especially in our tests there are many times we provide a value's that are "links" to the locale filesystem. My assumption of what's happening here is the file system "links" on windows all begin with file:// so they are considered valid, meanwhile on Linux and macOS they are just / which is invalid.

You can look at my commit where I use URL.canParse() religiously to ensure we never error out in these instances, and it does in fact have tests passing on every platform.

So please utilize the same methodology or else adding some try...catch blocks to fix, and we should be good to go!

[idleberg]

meanwhile on Linux and macOS they are just / which is invalid.

Isn't this an issue that should be fixed rather than working around it? Or is it a rabbit hole?

[savetheclocktower]

So it sounds like url.parse needs to be replaced with a helper function that

• detects paths on disk and handles them specially
• returns false if URL.canParse returns false
• passes the rest to new URL

[confused-Techie]

@savetheclocktower I'd personally disagree, but it still would be a valid solution.

The code is already written in such a way that it handles either a parsed URL or local filesystem.

Just currently it does something like this:

const { protocol } = new URL(thing);

if (protocol === "https:") {
  doThingWithUrl();
} else {
 doThingWithLocalFileSystem();
} 

But because this now throws an error, we just need to do:

if (URL.canParse(thing)) {
  const { protocol } = new URL(thing);

  if (protocol === "https:") {
    doThingWithUrl();
  }
} else {
  doThingWithLocalFileSystem();
}

So while we could add a helper function, the code already does this just fine. The only difference is now instead of getting a presumably null value for protocol on an invalid string, we get a thrown error.

[idleberg]

I have committed a change. However, I see room for improvement:

A successful URL validation using url.canParse() will end up parsing the URL a second time (new URL()). This is probably negligible, but fixed easily in a helper function. I was hesitant to introduce this, since there don't seem to be any cross-module helpers.

You choose which approach you prefer.

[confused-Techie]

Thanks a ton for addressing my feedback and getting this PR working!

I think this is a totally fine way to handle this. Although I'll give some time for @savetheclocktower to chime in if they still would prefer a helper function, which like you mentioned does improve the performance slightly, and if we wanted to add it, the closest thing PPM has to a cross module helper is likely apm.js which is commonly imported as config in the top level of many modules here.

[savetheclocktower]

I'm fine with this approach if you are. If there were more than four places that needed changing, I'd argue a helper function was warranted. Otherwise, since the specs pass, I'm 👍.

[confused-Techie]

Awesome, thanks for your feedback & thanks for your contribution!
Lets get this one merged!