Move read and write logic from nativeTextFileService to textFileService for #79275

[gyzerok]

Hey @bpasero! Here comes 😄

This PR isn't finished yet. I am opening it to get your early feedback and to ask some questions.

What is blocking

Currently I am blocked by #79275 (comment). I've made a PR for iconv-lite to fix the issue and am currently awaiting the resolution.

Also in case iconv-lite author will decide to reject my PR we can still proceed forward by allowing only subset of encodings in the browser for which we can ensure Uint8Array support by test coverage.

Tests

I was looking into how I could run tests for textFileService inside browser as well.

The problem there (as we discussed before) is that we can't load fixtures from files inside browser environment. So I figured I can pre-encode fixtures data and put it inside ts modules. Overall it worked great and even allowed me to discover an issue with jschardet I've missed in my previous PR.

I wrote the tests so that they repeat the same tests for nativeTextFileService exactly with the exception of the environment. So here comes options on which I seek your guidance:

Is loading from file system crucial for us when testing nativeTextFileService?

1. NO: We can actually remove these tests now and just leave those for textFileService which will run in both environments
2. YES: We move actual test cases to a separate file, then import it in both native and common tests populating environments differently. In this case test can be written only once.

Personally I like option 1 more. In my opinion we should test that textFileService works, not that the file system works. So in my mind loading fixtures from actual file system is not relevant for this tests.

Other questions

It looks like AbstractTextFileService only uses what is defined in common, but itself is in browser. Is it intentional? Should it be moved to common?

[bpasero]

@gyzerok

Also in case iconv-lite author will decide to reject my PR we can still proceed forward by allowing > only subset of encodings in the browser for which we can ensure Uint8Array support by test
coverage.

This would not be a good option going forward though. VSCode itself is exploring to enable a sandbox option from Electro that prevents any node.js code to be executed in the window. My hope was that the work you do can benefit both web and the future sandboxed desktop environment. I would want this solution to support all encodings in both web and sandboxed desktop if possible. If there is a reason we cannot support this for some encodings, we should understand why that is and come up with a custom solution if possible. I thought that iconv-lite through webpack would support all encodings because there are no native dependencies, but please correct me if that assumption was wrong. E.g. if we need node.js for the UTF-16 conversion, we should try to write our own converter if possible.

Is loading from file system crucial for us when testing nativeTextFileService?

Yeah I think for desktop I would like to preserve that because it will test the actual stack down to the disk. However, we have a way to register file system providers and we could simply swap out the disk based provider with an in-memory one here:

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

Line 54 in b8f63e9

disposables.add(fileService.registerProvider(Schemas.file, fileProvider));

In other words, all those tests could run fine in web if the file system provider registered is one that is in-memory for web. For example we have one here:

vscode/src/vs/platform/files/common/inMemoryFilesystemProvider.ts

Line 53 in b8f63e9

export class InMemoryFileSystemProvider extends Disposable implements IFileSystemProviderWithFileReadWriteCapability {

It looks like AbstractTextFileService only uses what is defined in common, but itself is in browser. > Is it intentional? Should it be moved to common?

It cannot move currently due to:

vscode/src/vs/workbench/services/textfile/browser/textFileService.ts

Line 32 in b8f63e9

import { ICodeEditorService } from 'vs/editor/browser/services/codeEditorService';

[gyzerok]

@bpasero

I thought that iconv-lite through webpack would support all encodings because there are no native dependencies, but please correct me if that assumption was wrong.

Your assumption is correct. It's not about native dependencies, it's about iconv-lite relying on the fact that it gets Buffer as input and not Uint8Array. The solution is very straightforward - adding Buffer.from in couple places. So in the worst case scenario we can always go with a fork.

Yeah I think for desktop I would like to preserve that because it will test the actual stack down to the disk.

👍

However, we have a way to register file system providers and we could simply swap out the disk based provider with an in-memory one here

Yeah, that's exactly what I am doing currently. Will adjust then to reuse the same tests in-between environments with different file services.

[bpasero]

Converting to draft, simply ping when I should give a review.

[gyzerok]

@bpasero at last I am ready for the first full review! 🎉

[bpasero]

Great, but as we enter endgame for June, this will slip to next week and July release.

[bpasero]

@gyzerok overall this looks very good, I pushed few cosmetic changes via 82ca79b if you want to check.

Two things come to my mind though:

• running tests from your branch (even before my changes) results in a lot of test failures (via yarn test-browser). I cannot really explain that given the build also runs these tests and seems to be green. I ran them on macOS, do you see that too?
• there seems to be a questionable check for value that does not account for the empty string [1]

[1] https://github.com/gyzerok/vscode/blob/82ca79b0825ff59c4f1a18b95a9aa649cd76a59e/src/vs/workbench/services/textfile/browser/textFileService.ts#L170

[bpasero]

Also going forward, would be great to add individual commits on top and not squash so that reviewing becomes easier 👍

[gyzerok]

@bpasero especially love your changes around timeout. They make code much more linear! Didn't know this function exists.

I rebased on the latest master and made couple of fixes.

By the way how do you make these cool code inline blocks in your comments?

running tests from your branch (even before my changes) results in a lot of test failures (via yarn test-browser). I cannot really explain that given the build also runs these tests and seems to be green. I ran them on macOS, do you see that too?

Do the errors you are seeing look like assertion error between array converted to string and a string? If so this is due to iconv-lite-umd being pre 0.6.5. The funny thing is - I am 100% sure I ran tests locally before pushing and they worked.

Also I can clearly see exactly these tests passing in Azure. And there is no way they can pass in browser if iconv-lite-umd is not 0.6.5.

My guess is as follows:

1. I've updated master to 0.6.5.
2. Then I've switched to my own branch, but forgot to rebase on new master => branch was still on 0.6.4, but my node_modules were on 0.6.5.
3. So locally everything worked for me and my local test runs were also successful.
4. In Azure dependencies for my branch got populated with the cache from master.

So I suggest you to check the Azure. If I am correct than it doesn't seem to me like a desired cache restoration behavior.

there seems to be a questionable check for value that does not account for the empty string

Haha, great catch! I feel ashamed by the errors I am making 😅 Huge thanks for your thorough reviews, which helps us find these oversights! 👍

[bpasero]

@gyzerok

especially love your changes around timeout. They make code much more linear! Didn't know this function exists.

👍 , I think even before you did call setTimeout in a for loop so this test was not actually delaying chunks by a timeout, but simply sending them into the stream all at once after a timeout. With my change it also now waits for one event loop before pushing more data into the stream.

By the way how do you make these cool code inline blocks in your comments?

You probably mean the code references. Simply open a file on GH, go to the line (you can even select multiple lines when you shift-click) and copy the "Permalink". If you paste a permalink into a comment, GH will show the sources.

Do the errors you are seeing look like assertion error between array converted to string and a string? If so this is due to iconv-lite-umd being pre 0.6.5. The funny thing is - I am 100% sure I ran tests locally before pushing and they worked.

I can check again, meanwhile can you make a git clean -xfd && yarn && yarn watch in your branch and run yarn test-browser to see the test failures?

Haha, great catch! I feel ashamed by the errors I am making 😅 Huge thanks for your thorough reviews, which helps us find these oversights! 👍

No worries, it was wise to do these changes through various PRs to keep the review at a manageable level 👍

[gyzerok]

@bpasero

I can check again, meanwhile can you make a git clean -xfd && yarn && yarn watch in your branch and run yarn test-browser to see the test failures?

I've already done that when fixing. It should work for you as well if you update the branch to the latest version.

[gyzerok]

I think even before you did call setTimeout in a for loop so this test was not actually delaying chunks by a timeout, but simply sending them into the stream all at once after a timeout. With my change it also now waits for one event loop before pushing more data into the stream.

I’ve expected it to send each chunk in a separate event loop frame, but after the loop (current frame) is finished, yes.

Can you widen my knowledge here? Why would they go all in a single frame?

[bpasero]

Confirmed, works fine now, good job 👍 . Thanks a ton for making these tests run inside browser environment.

[bpasero]

I’ve expected it to send each chunk in a separate event loop frame, but after the loop (current frame) is finished, yes.
Can you widen my knowledge here? Why would they go all in a single frame?

I am not an expert in what the browser internally does, so I was speculating that await timeout(0) would behave differently from repeated setTimeout() calls. E.g. I would think:

• awaiting each iteration means a chunk is only added to the stream one by one
• repeated setTimeout has a chance of adding all chunks at once before the next event loop starts in one go

In other words, awaiting in each iteration seems closer to reality, e.g. when comparing with disk IO.

[bpasero]

@gyzerok merged! I guess only missing now is to enable encoding related UI for web 🚀