Fix Port message loss before attachFS is called

[UnCor3]

Thank you for your response, here is what i came up with

• Added resolveRemoteMount
• Added test for resolveRemoteMount (used node worker as jest-worker seems to be using node workers under the hood)

instead of implementing port.emit('message', data) called handleRequest directly as it's what attachFS would do

export function catchMessages<T extends Backend>(port: Port): (fs: FilesystemOf<T>) => void {
	const events: _MessageEvent[] = [];
	const handler = events.push.bind(events);
	attach(port, handler);
	return function (fs: any) {
		detach(port, handler);
		for (const event of events) {
			const request: FileOrFSRequest = 'data' in event ? event.data : event;
			handleRequest(port, fs, request);
		}
	};
}

returned the fs that's resolved from awaiting resolveMountConfig and added proper types

export async function resolveRemoteMount<T extends Backend>(port: RPC.Port, config: MountConfiguration<T>, _depth = 0): Promise<FilesystemOf<T>> {
   const stopAndReplay = RPC.catchMessages(port);
   const fs = await resolveMountConfig(config, _depth);
   attachFS(port, fs);
   stopAndReplay(fs);
   return fs;
}

finally the tests look like this, i named the files with .browser extension lmk if you'd like me to change

worker.browser.js

import { parentPort } from 'node:worker_threads';
import { resolveRemoteMount } from '../../dist/backends/port/fs.js';
import { InMemory } from '../../dist/backends/memory.js';

await resolveRemoteMount(parentPort, { backend: InMemory });

remote.browser.test.ts

import { dirname } from 'node:path';
import { fileURLToPath } from 'node:url';
import { Worker } from 'node:worker_threads';
import { Port } from '../../src/backends/port/fs.js';
import { configureSingle, fs } from '../../src/index.js';

const dir = dirname(fileURLToPath(import.meta.url));

let port: Worker;

try {
	port = new Worker(dir + '/worker.browser.js');
} catch (e) {
	/* nothing */
}

describe('Remote FS with resolveRemoteMount', () => {
	const content = 'FS is in a port';

	test('Build exists for worker', () => {
		expect(port).toBeDefined();
	});

	(port ? test : test.skip)('Configuration', async () => {
		await configureSingle({ backend: Port, port, timeout: 300 });
	});

	(port ? test : test.skip)('Write', async () => {
		await fs.promises.writeFile('/test', content);
	});

	(port ? test : test.skip)('Read', async () => {
		expect(await fs.promises.readFile('/test', 'utf8')).toBe(content);
	});

	(port ? test : test.skip)('Cleanup', async () => {
		await port.terminate();
		port.unref();
	});
});

[james-pre]

Looks good. I made some comments about minor things, but overall I think this PR is solid. Sorry for taking so long to get back to you.

For the test names, I would prefer if it didn't include browser in the name. After all, the tests are actually run in Node. Perhaps naming it config.test.ts/config.worker.js would be more clear as to what the test is for?

[james-pre]

_depth is an argument used internally by resolveMountConfig to track recursive calls and avoid infinite loops. I don't think it should be used here, though it could be helpful for debugging.

[james-pre]

I hope you don't mind, I made some of the minor changes I mentioned.

If you don't mind answering, is there a reason you opened a new PR vs opening the old one?

Anyway, thanks again for helping out with this.

instead of implementing port.emit('message', data) called handleRequest directly as it's what attachFS would do

This is a much better solution than what I did in my suggestion, just thought I'd point that out =)

[james-pre]

For future contributors:

This PR is a continuation of #92, with some suggestions. It should resolve #91.

It fixes an issue where Port would throw an "RPC failed" error due to message loss occurring in the worker before attachFS was called.

[UnCor3]

If you don't mind answering, is there a reason you opened a new PR vs opening the old one?

There wasn't a specific reason just wanted to do so :D

This is a much better solution than what I did in my suggestion, just thought I'd point that out =)

Yeah you did point it out really

For the test names, I would prefer if it didn't include browser in the name. After all, the tests are actually run in Node. Perhaps naming it config.test.ts/config.worker.js would be more clear as to what the test is for?

Just changed them I think this pr is done now

Sorry for taking so long to get back to you.

No worries....

[james-pre]

Everything looks good, I'll merge it. Thanks again, I really appreciate contributions!