Avoid __nodeRequire()

[bpasero]

We still have a few places of a __nodeRequire that we should convert into async import().

For example, code such as:

this._keytar = <IKeytarModule>require.__$__nodeRequire('keytar');

becomes:

this._keytar = await import('keytar');

Places:

• MainThreadKeytar @chrmarti
• readWindowsCaCertificates @chrmarti
• TerminalInstanceService @Tyriar
• RawDebugSession @isidorn
• Win32UpdateService @joaomoreno
• RemoteAgentConnection @alexandrudima

[isidorn]

@weinand actually added the dependency to vsda. However when I change the code to await import('vsda'); I get an error that vsda can not be found. @weinand should we specify that we use vsda somewhere?
Code pointer https://github.com/Microsoft/vscode/blob/a00a9a29452546c6f5018f89d5c9f051f841dd38/src/vs/workbench/contrib/debug/electron-browser/rawDebugSession.ts#L531

[joaomoreno]

Isn't this still going sync, deep down?

[weinand]

@isidorn __$__nodeRequire comes from the vscode.loader. The owner of that package (@alexandrudima or @jrieken) should advise how to replace this with an async import.

BTW, 'vsda' is used in two places: RawDebugSession and RemoteAgentConnection. Please fix both of them since Alex is out.

[bpasero]

@isidorn yeah that sucks, we probably need to throw in a vsda.d.ts into src/typings to make TS happy :-|

[bpasero]

Updated the title, there is no difference in loading behaviour but I feel __nodeRequire() is really a hack from the times where we could not write async import(...). It also gives us proper typing.

[jrieken]

Isn't this still going sync, deep down?

Yeah, it's still equally sync. Writing async import 'foo' is the same as writing new Promise(resolve => load_module('foo')). It still helps because it auto-generates load_module for you.

I get an error that vsda can not be found. @weinand should we specify that we use vsda somewhere?

Putting declare module 'vsda' {} anywhere in your code should help

[isidorn]

Thanks for advice.
Added vsda.d.ts and fixed the issue in rawDebugSession
Now strangly the RemoteAgentConnection can not await import('keytar'); since that import violates the ts lint import rule. @bpasero should I put the vsda.d.ts in some other typings folder, since the RemoteAgentConnection can not depend on it from src/typings?

[bpasero]

It should be able to depend, then we need to tweak some tslint rule maybe.

[isidorn]

Ok. I have tweaked the tslint rule to allow vsda to be imported. Did not use a * to not allow random node modules to be imported there.
Seems to work fine now

[bpasero]

@isidorn looks like this is causing build failures and they can be explained by the fact that remoteAgentConnection is living in common when it should live in node. I am not sure why that is, maybe we should leave that one up for next milestone and move it properly.

[isidorn]

@bpasero yup, just saw the build failure. Reverting my commit. Let's tackle in June.

[Tyriar]

I'll be replacing these with importing standalone modules soon:

vscode/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts

Lines 37 to 38 in 6526507

	Terminal.applyAddon(require.__$__nodeRequire('vscode-xterm/lib/addons/search/search'));
	Terminal.applyAddon(require.__$__nodeRequire('vscode-xterm/lib/addons/webLinks/webLinks'));

[Tyriar]

Terminal ones removed in #74858

[isidorn]

@bpasero I can look into moving remoteAgentConnection to node.
@aeschli does this make sense to you? I see some modules from common depend on it, so I wold probalby split the remoteAgentConnection to have the implementation in node and interfaces in common

[chrmarti]

@bpasero Do you know why mainThreadKeytar.ts was placed in the 'browser' and not the 'node' folder? Maybe that was an oversight.

[isidorn]

@bpasero I looked into the remoteAgentConnection and it is a bit more complex. The issue is that it can not be nicely split up between common and node since the actual function that uses require.__$__nodeRequire syntax is used by other modules not living in node.
We seem to use the vsda module for signing when in the native environment. And due to that I believe we need two implementations of connectToRemoteExtensionHostAgent one for the native and one for the rest. So we basically get rid of this check.

I could also pass in the signer as part of the options, so the remoteAgentConnection stays in common and different clients would pass a signer.

[bpasero]

@chrmarti I noticed this and it is a bad hack, but I am not sure why that was done. The right solution would be to introduce a service for this and implement it in browser and node environments.

@isidorn I do not understand

[isidorn]

fyi
There is a difference in the behaivor of the __nodeRequiere() and await import('')
When the module is missing, the second pattern will only throw the error the first time you import the moduel.
Due to that I had to cache the promise to always have the expected behavior for the missing module

https://github.com/Microsoft/vscode/blob/18f4630bc51a3d87c7b31d96ca9c8ea95aa649b0/src/vs/platform/sign/node/signService.ts#L15

[isidorn]

Created #75257 as a follow up