Move encoding to common for #79275

[gyzerok]

Next PR in the series. Super excited that we are moving forward! 😄

I was thinking if I should move encoding to common or browser. According to your wiki, to go to browser modules have to use some browser-specific APIs, which as I understand it is not the case here.

What would be the best way to fix Monaco build errors?

It would probably make sense to merge #100541 first.

[bpasero]

@gyzerok left some comments inside.

I was thinking if I should move encoding to common or browser. According to your wiki, to go to browser modules have to use some browser-specific APIs, which as I understand it is not the case here.

I think common is fine however as long as we use Buffer we cannot move it to this layer. So this needs to be cleaned up first.

What would be the best way to fix Monaco build errors?

I think we should probably move encoding out of vs/base and into src/vs/workbench/services/textfile/common/encoding.ts. Imho the workbench is the only client and as such the monaco standalone editor will not accidentally include it.

[gyzerok]

@bpasero as you mention in the issue thread we can just change signatures to expose Uint8Arrays. I've looked into it a bit closer and it seems to be the best idea 😄

1. The whole vscode test suit around encodings runs fine if we remove Buffer.from. I also checked that Buffer.isBuffer returns false inside. So we indeed pass Uint8Array there.
2. According to iconv-lite author it should work with Uint8Arrays just fine.
3. I've looked through the iconv-lite source and didn't find any non-Uint8Array method usages within decoders.

Should I proceed with it?

[bpasero]

Yeah sounds good 👍

[bpasero]

@gyzerok looks like merge conflicts, let me know once I should review again

[gyzerok]

Yeah, sure. I am waiting on the last piece aadsm/jschardet#58

[bpasero]

@gyzerok I believe jschardet is unfortunately not active anymore, so I would go ahead and proceed without your change by doing some any tricks in TypeScript.

Alternatively we might have to fork it as vscode-jschardet.

[gyzerok]

@bpasero yes, applying any in such case is what I would do in my own codebase. Was afraid you won't accept it as a solution. Done now.

One question still I want to double check with you - should the tests for encoding.ts be now moved under workbench or kept where they are?

[bpasero]

@gyzerok

One question still I want to double check with you - should the tests for encoding.ts be now moved under workbench or kept where they are?

If encoding.ts ends up to be in workbench, the tests should just follow, e.g. into the tests folder of textfiles

[gyzerok]

@bpasero all done now.

It looks like tests stopped working locally due to some changes in master.

(electron) The default value of app.allowRendererProcessReuse is deprecated, it is currently "false".  It will change to be "true" in Electron 9.  For more information please check https://github.com/electron/electron/issues/18397
(node:13021) Electron: Loading non-context-aware native module in renderer: '/Users/gyzerok/Code/private/vscode/node_modules/vscode-nsfw/build/Release/nsfw.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
{"phase":"loading","moduleId":"vscode-nsfw","neededBy":["vs/platform/files/node/watcher/nsfw/nsfwWatcherService"]}

[bpasero]

@gyzerok this typically means you should do a round of git clean -xfd && yarn && yarn watch to ensure you install latest dependencies (save your work first, git clean might wipe it).

The changes look pretty good to me, only thing I noticed is that the encoding tests should live inside the node folder. Can you simply rename src/vs/workbench/services/textfile/test/encoding to src/vs/workbench/services/textfile/test/node?

[bpasero]

I think there is also a compile error here:

vscode/src/vs/workbench/services/textfile/test/electron-browser/textFileService.io.test.ts

Line 28 in aa06a3d

import { detectEncodingByBOM } from 'vs/base/test/node/encoding/encoding.test';

[gyzerok]

@bpasero yeah, noticed. Sorry for that. Now it's under node as it should.

[gyzerok]

And compilation error fixed as well. Sometimes yarn watchd doesn't find them. Not sure what the problem is...

[bpasero]

@gyzerok great, one thing I noticed is that you added the modules to workbench-dev.html but not workbench.html. I mean these lines:

'iconv-lite-umd': `${window.location.origin}/static/remote/web/node_modules/iconv-lite-umd/lib/iconv-lite-umd.js`,
'jschardet': `${window.location.origin}/static/remote/web/node_modules/jschardet/dist/jschardet.min.js`,

[gyzerok]

@bpasero are you talking about https://github.com/microsoft/vscode/pull/100539/files#diff-83d3bfc25cd4ba539f4d26e3047d7b1bR26-R27 or am I missing something else?

[gyzerok]

Github seems to scroll past the line when going to the link here is the one to the file change: https://github.com/microsoft/vscode/pull/100539/files#diff-83d3bfc25cd4ba539f4d26e3047d7b1b

[bpasero]

@gyzerok yeah so workbench-dev.html is running when running out of sources and workbench.html from a build (something I can test locally). You correctly added the prefetch to the workbench.html but we also need the loader configuration to match:

self.require = {
	baseUrl: `${window.location.origin}/static/out`,
	paths: {
		'vscode-textmate': `${window.location.origin}/static/node_modules/vscode-textmate/release/main`,
		'vscode-oniguruma': `${window.location.origin}/static/node_modules/vscode-oniguruma/release/main`,
		'xterm': `${window.location.origin}/static/node_modules/xterm/lib/xterm.js`,
		'xterm-addon-search': `${window.location.origin}/static/node_modules/xterm-addon-search/lib/xterm-addon-search.js`,
		'xterm-addon-unicode11': `${window.location.origin}/static/node_modules/xterm-addon-unicode11/lib/xterm-addon-unicode11.js`,
		'xterm-addon-webgl': `${window.location.origin}/static/node_modules/xterm-addon-webgl/lib/xterm-addon-webgl.js`,
		'semver-umd': `${window.location.origin}/static/node_modules/semver-umd/lib/semver-umd.js`,
		'iconv-lite-umd': `${window.location.origin}/static/node_modules/iconv-lite-umd/lib/iconv-lite-umd.js`,
		'jschardet': `${window.location.origin}/static/node_modules/jschardet/dist/jschardet.min.js`,
	}
};

[gyzerok]

@bpasero trueee. Fixed it as well.

Good think you are checking. Seems like I am missing some small but crucial details all the time. Hopefully it will wear off, after I learn vscode codebase a bit more :)

Also your solution for tests worked like a charm. Thank you! 👍

[bpasero]

Great stuff, merged. I will trigger a build to do some more testing, feel free to go ahead with the next part of enabling this for web and pushing down the encoding handling to the abstract textfileservice.