Put together a net module
#1
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The changes here bind to the node.js `net` module. The API tries to stay true to the node.js API while still being usable. There are a couple of places where things are a bit inconsistent. The naming for `onCloseServer` and `onCloseSocket`, `onErrorServer` and `onErrorSocket`, and `localAddressOption` and `localPortOption`. This might be showing that there is a split waiting to happen. Maybe we should have `Node.Net`, `Node.Net.Server` and `Node.Net.Socket`? Maybe it doesn't matter that much? In any case, this gives us bindings to the `net` module!
joneshf
commented
May 9, 2019
hdgarrood
reviewed
May 11, 2019
We had a couple of outliers where we had to suffix them with `Option`. For consistency sake, we switch the scheme to prefixing them with the type of option they are.
From feedback, we can avoid some allocations if we do the uncurrying explicitly. This cleans up the FFI implementation a bit at the expense of the PS. That seems like a fair trade-off since PS is easier to maintain than JS. There are two bugs that got fixed in this change as well. The first is that we were typing `Lookup` incorrectly. The `family` is a `Maybe Int`, not a `Maybe String`. This was silently failing but not being caught for whatever reason. There is some kind of concurrency issue with all the callbacks. The second is that for `Effect _`, `ado` generates different code from `do`. The difference here means we get the inefficient version of the code. Rather than use `ado` and suffer the penalty, we move over to `do` and allow for more efficient code.
All of the event handlers had the same format, and really were doing the same thing. There's a reason that `EventEmitter#on` is a thing. We can use that to our advantage and write less JS!
Per review, using unqualified imports tends to lead to merge conflicts. We can mitigate that by qualifying imports.
Per review, we can drop the namespace on some values if we separate modules. Having done the move, it's not clear that the extra level of nesting was worth it. It was one or two values that needed a suffix. But now we've got a whole new module and another file of JS to maintain. Oh well, we're here now.
This is the second part of movind to modules. Again, not sure this is worth it as we end up with more code to maintain and a less clear API overall. We should have left well-enough alone and had six out of 78 concepts in this module inconsistent. Oh well, maybe someone will find a better set of values in the future.
|
I think I hit all the comments. Mind giving it another look over? Also, any idea how to get CI working? |
|
I think CI will "just work" as soon as there's a |
|
Scratch that, I just found the switch I needed to toggle in the travis ci web interface. It should build the next time you push. I'll re-review in a little bit. |
We interpreted the node.js documentation for the `'drain'` event. Attaching the listener doesn't return a `Boolean`; it returns the `Socket` like all other `#on` calls. Since we're not returning the `Socket` for any other listeners, we don't want to do it here.
|
I've added a |
|
@hdgarrood Bumping in case this dropped off your plate. |
hdgarrood
approved these changes
May 26, 2019
Per review, this syntax should be a bit clearer.
|
Thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This throws together a first pass at binding to the node.js
netmodule. I tried to follow the style of the other packages we have, but let me know if I'm missing anything.The commit has some comments, but I'm going to make a review so they're contextual.
Also, how do we enable travis for this repo? I don't see a way to turn on the app in the settings, and I don't have access through travis-ci.org.