-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
31ab0de
to
b452d37
Compare
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. |
I'm cool with that |
b452d37
to
29d442c
Compare
Splits out the ethernet / tcp emulation logic from the fetch networking adapter for reuse in wisp.
29d442c
to
3490e5d
Compare
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. |
tools/fetch_network_harness.js
Outdated
|
||
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")); |
There was a problem hiding this comment.
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?
- 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
- 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 themodule.exports
that are only used for tests, and makes the loading mechanism the same for browser and nodejs - Keep all the networking code in the same file
I prefer 1. over 2. and 2. over 3., but I'm open to suggestions.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
016dc5e
to
6c917c6
Compare
Thanks! |
Splits out the ethernet / tcp emulation logic from the fetch networking adapter for reuse in wisp.