Support JavaScript URL imports

[MarkTiedemann]

See: #613

Looking for feedback, @stefanbuck.

[MarkTiedemann]

Not sure what's wrong with the snapshots, yarn test worked locally. Can you take a look, @stefanbuck?

[stefanbuck]

First, thanks for taking the time to submit this feature.

The snapshot is failing on Travis, because by default Jest does not write and snapshot changes when running on CI. I checked out your branch locally and running yarn test adds this snapshot.

+exports[`normaliseResolverResults converts [object Object] 1`] = `
+Array [
+  Object {
+    "registry": "npm",
+    "target": "foo",
+    "type": "registry",
+  },
+]
+`;

I wonder why this changed. Anyway, maybe best to just revert the snapshot change which solved the issue for me.

I'll have a closer look at the other changes later today.

Just one thing upfront, do you mind to add an End2End test or two for this change. It's super easy to add and described here
https://github.com/OctoLinker/OctoLinker/tree/master/e2e Thank you

[stefanbuck]

I left one tiny comment related to the url handling, but apart from that this looks good to me.

[MarkTiedemann]

Not sure how to fix this. ^

[MarkTiedemann]

Anyway, maybe best to just revert the snapshot change which solved the issue for me.

Sure, will do.

do you mind to add an End2End test or two for this change

No, I didn't know you had e2e tests. Will add some tomorrow!

but apart from that this looks good to me.

Awesome. :)

[MarkTiedemann]

I deleted the snapshot file, then re-ran yarn test - but there's no diff. Am I missing anything on how snapshots work? I don't have any experience with them.

Also, if I undo the snapshot changes manually, and re-run yarn test -u, I still end up with the same result.

[MarkTiedemann]

I added 1 simple e2e test - I hope that's fine.

PS: Many props to you for making it so simple to add an e2e test, and to the great documentation in general! :)

[stefanbuck]

I added two addtional commits on top. The first is reverting the snapshot (I have no idea what is happening on your end). Anyway, let's see if this work on Travis. The other is using is-url which also enabled to fix the other test.

[stefanbuck]

Mhhh still failing 😕 The latest master is passing so something must be odd with this branch. I'll give it another try tomorrow. What do you think about the other changes I made?

[MarkTiedemann]

Mhhh still failing 😕 The latest master is passing so something must be odd with this branch. I'll give it another try tomorrow.

That is weird indeed. At the weekend, I would have time to debug as well.

The other is using is-url which also enabled to fix the other test.

I think using is-url is fine, but I'd prefer new URL() so we can ship less code (with 94%, browser support is pretty good).

[stefanbuck]

Looks like the last commit 4d132be fix it. My other commit form yesterday 0cd1aae was intended to do the same, but I failed 😄 Anyway, all good now.

Regarding the is-url dependency. I'm less worried about the size since it's a browser extensions and doesn't really matter if it's 10kb bigger or not. However, I went with URL approach and simplified it slightly. As mentioned earlier, this does not 100% match the spec since it also link just a domain, but that's fine in my opinion.

[MarkTiedemann]

Anyway, all good now.

Awesome! Thank you for fixing the snapshots!

I'm less worried about the size since it's a browser extensions

True.

I think a better reason for using new URL() rather than is-url is that it's a standard rather than just some NPM package. So, in theory, it should be easier to understand and maintain this code.

As mentioned earlier, this does not 100% match the spec since it also link just a domain, but that's fine in my opinion.

I agree, that's fine.

Again, thanks a lot!

[MarkTiedemann]

Is this ready to be merged yet, @stefanbuck? Or is there anything I still need to do?

[stefanbuck]

Nope, all good. I will release a new version later this weekend.

[stefanbuck]

Thanks again for adding this feature