Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

defaultWorkerFactory.ts: deprecated require.toUrl() use #107440

Closed
bpasero opened this issue Sep 25, 2020 · 3 comments
Closed

defaultWorkerFactory.ts: deprecated require.toUrl() use #107440

bpasero opened this issue Sep 25, 2020 · 3 comments
Assignees
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders sandbox Running VSCode in a node-free environment
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Sep 25, 2020

The context is: #107437 and the deprecation of require.toUrl

I noticed 2 uses of require.toUrl that ideally we can replace with FileAccess.asFileUri and FileAccess.asBrowserUri:

const workerMain = require.toUrl('./' + workerId);

const workerBaseUrl = require.toUrl(myPath).slice(0, -myPath.length);

I would have done it but wasn't exactly sure, my guess would be to use the FileAccess.asBrowserUri variant here since we are in browser context.

@bpasero bpasero added debt Code quality issues sandbox Running VSCode in a node-free environment labels Sep 25, 2020
@alexdima
Copy link
Member

alexdima commented Sep 25, 2020

I'm a bit concerned about changing this, as this piece of code runs in the standalone editor too. So before this piece of code would go directly to the loader (which could be require.js and not our loader) and get back a string.

With the proposed change to use FileAccess.asBrowserUri, that string must now always go through our URI.parse and I believe URI.parse(str).toString() is not guaranteed to be equal to str.

@bpasero
Copy link
Member Author

bpasero commented Sep 25, 2020

@alexdima understood and that is also the reason why I wanted to ask you about it since I was not so sure.

When I test from the vscode-file branch it seems to me the worker is coming up properly, at least on macOS:

image

So it may well be that things continue to work, either with file or vscode-file protocol because we load the worker in a browser context always.

I am fine leaving this use as is and maybe add a comment. I just wanted to bring this up to ensure this continues to work, even when we switch to the new base URL with vscode-file://vscode-app/... structure.

@bpasero bpasero added this to the October 2020 milestone Sep 29, 2020
@bpasero bpasero closed this as completed in 19c3db8 Oct 5, 2020
@bpasero
Copy link
Member Author

bpasero commented Oct 5, 2020

I just added a comment explaining the use.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders sandbox Running VSCode in a node-free environment
Projects
None yet
Development

No branches or pull requests

3 participants
@bpasero @alexdima and others