Incompatibility with JupyterLab core npm packages 3.0.10

[bsyouness]

Description

We use a custom version of JupyterLab which depends on the following dependencies that I upgraded:

Doing that broke the LSP extension in that the check if not uri.startswith(virtual_documents_uri) in "virtual_documents_shadow.py" at line 141 now fails. This is due to an additional "/" that gets inserted in the file URI as per this example:

uri file://c:/XXX/.virtual_documents/1%20Migration%20Guide.ipynb doesn't start with file:///c:/XXX/.virtual_documents

Reproduce

Bump up to the latest version of JupyterLab.

[krassowski]

Thank you for the heads up! Would you like to open a PR to fix this (and bump the versions?).

[bsyouness]

Sure yeah!

[krassowski]

It would be very welcome :) It seems to be related to url-parse 1.5.1 update and I think that is causing the current failures..

[bsyouness]

So we've been looking into this and trying to make sense of it... Should we be using 3 or 2 slashes. 3 slashes is windows-specific and was the status-quo before this breaking change. Do we want to stick with 3 slashes and amend the way the URI is built in jupyterlab core somehow? Any other suggestions?

[krassowski]

@bollwyvl do you have an opinion on this one?

[bollwyvl]

Aw, man, I sure love windows paths. LOVE'm. It's unlikely i'll have any bandwidth to work on this this week, but one of them has to be right. Looks like the python tests expect three slashes. This jives with my understanding of the // being the protocol separator, and then the <nothing here>/ being the authority.

[krassowski]

unshiftio/url-parse#203 looks like an upstream bug report for this issue.

[bollwyvl]

Whew, saved. So if anything, we need to specify _not_ 1.5.1, regex ddos be damned?

[krassowski]

I think the troublesome fix went into 1.5.0. It might be difficult to revert to an earlier version since it got a) a security fix label b) already merged and released by JupyterLab core. I was thinking about an upstream fix, or making our server side comparison be more lenient (as a special case for the lack/presence of triple /).

[krassowski]

1.5.1 was meant to address some of the issues introduced by the fix but not this one. I will make some noise on the issue.

[bsyouness]

I think the troublesome fix went into 1.5.0. It might be difficult to revert to an earlier version since it got a) a security fix label b) already merged and released by JupyterLab core. I was thinking about an upstream fix, or making our server side comparison be more lenient (as a special case for the lack/presence of triple /).

I think maybe the latter (making the server side comparison more lax), would be a good idea too in case something like this happens again — according to the issue you identified I get the impression that this kind of url parsing will remain problematic. I'm happy to create a PR to that effect if you want to tell me how you think the comparison should be performed.

[krassowski]

Maybe just strip the prefix before preforming the starts with check, whether it is file:// or file:///?
Regular expression would be an alternative, but maybe better to avoid it.

[bollwyvl]

URI is a for-real spec: https://datatracker.ietf.org/doc/html/rfc3986 A number of language servers are real picky about actually implementing it correctly, down to the correct capitalization of the drive letter on windows, preceded by three slashes, or they will not find your file. I don't think we can budge much on our client and server agreeing 100%, and robustly testing. So pretty much we need to either find a replacement library, or work with the upstream, and get some of these degenerate cases into their test suite.