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

Split out fake networking from fetch logic. #1113

Merged
merged 3 commits into from
Aug 21, 2024

Conversation

basicer
Copy link
Contributor

@basicer basicer commented Aug 5, 2024

Splits out the ethernet / tcp emulation logic from the fetch networking adapter for reuse in wisp.

@basicer basicer force-pushed the split-fake-networking branch 8 times, most recently from 31ab0de to b452d37 Compare August 5, 2024 09:01
@ProgrammerIn-wonderland
Copy link
Contributor

This may be a bit duplicate to my PR since I also split it over there in https://github.com/ProgrammerIn-wonderland/v86-wisp-net/blob/master/src/browser/net_utils.js

@basicer
Copy link
Contributor Author

basicer commented Aug 5, 2024

This may be a bit duplicate to my PR since I also split it over there in https://github.com/ProgrammerIn-wonderland/v86-wisp-net/blob/master/src/browser/net_utils.js

That's intentional. As I mention on Gitter I think its best to split this out in its own PR. I want to make some improvements to the net code in general but don't want to create a giant merge conflict. Ideally we can get his merge quicky and you can rebase on top of it.

@ProgrammerIn-wonderland
Copy link
Contributor

I'm cool with that

Splits out the ethernet / tcp emulation logic
from the fetch networking adapter for reuse
in wisp.
@ProgrammerIn-wonderland
Copy link
Contributor

Not sure if it makes sense to put the fake DNS in there since it can only return one address and is therefore somewhat specific to fetch_networking, I can't see any way I can reuse that or handle_fake_networking since handle_fake_networking directly calls the fake DNS resolver

@basicer
Copy link
Contributor Author

basicer commented Aug 6, 2024

Not sure if it makes sense to put the fake DNS in there since it can only return one address and is therefore somewhat specific to fetch_networking, I can't see any way I can reuse that or handle_fake_networking since handle_fake_networking directly calls the fake DNS resolver

I was thinking when you make your PR based on this, you add the http based dns logic here, and add an option to enable it in the adapter.


const { fake_tcp_connect, handle_fake_networking } = require(path.join(__dirname, "..", "src", "browser", "fake_network.js"));
globalThis.fake_tcp_connect = fake_tcp_connect;
globalThis.handle_fake_networking = handle_fake_networking;

const { FetchNetworkAdapter } = require(path.join(__dirname, "..", "src", "browser", "fetch_network.js"));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this stuff (using globalThis, module.exports that ends up in the optimised build) is getting ugly, can we look for an alternative?

  1. Start using es6 imports/exports. I believe closure compiler supports it and will remove it from optimised build, but it needs checking. We could start by only using it for fake_tcp_connect, handle_fake_networking and FetchNetworkAdapter
  2. Accept that we're not using proper imports/exports and use vm.runInThisContext (just for this test). While still ugly, at least this allows getting rid of the module.exports that are only used for tests, and makes the loading mechanism the same for browser and nodejs
  3. Keep all the networking code in the same file

I prefer 1. over 2. and 2. over 3., but I'm open to suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be down to give retrofitting everything to use es6 a go in a separate PR. The other perk of this is I think we get rid of the several places we have a list of all the various js files.

Currently when developing I have a test file I use that uses vm.runInThisContext to let node load the project to work around this. If we just switched everything to use modules node would just be able to load it natively. Im not sure where native browser module support is these days, but I'd think debug.html would still work in most browsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I took a stab at 1, and it didn't work out that well. I got everything to compile, and even got eslint to pass with no-undef turned on. My biggest worry was that closure no longer seemed to understand that DEBUG was a define, and I was worried it was going to stop optimizing those out. There were also a few other assorted errors, and I was a bit annoyed I had to import files just to get their types, as there didn't seem to be a type import you could do like typescript.

Instead I polished up my loader and went for option two. This gets the tests access to pretty much any symbol and also allows development in node without running the closure compiler every change.

@copy copy merged commit 46366e2 into copy:master Aug 21, 2024
3 checks passed
@copy
Copy link
Owner

copy commented Aug 21, 2024

Thanks!

@copy copy mentioned this pull request Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants