← 输入框|github.com|刷新
Skip to content

Put together a net module - #1

Merged
joneshf merged 10 commits into
masterfrom
joneshf/initial-work
May 26, 2019
Merged

joneshf merged 10 commits into
masterfrom
joneshf/initial-work

Conversation

@joneshf

@joneshf joneshf commented May 9, 2019 •

Copy link
Copy Markdown
Collaborator

This throws together a first pass at binding to the node.js net module. 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.

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!
Comment thread src/Node/Net.purs Resolved
Comment thread src/Node/Net.purs Resolved
Comment thread src/Node/Net.purs Resolved
Comment thread src/Node/Net.purs Resolved
@joneshf
joneshf requested review from garyb and hdgarrood May 9, 2019 17:38
@joneshf
joneshf marked this pull request as ready for review May 9, 2019 17:38
Comment thread test/Test/Main.purs Resolved
Comment thread src/Node/Net.purs Resolved
Comment thread src/Node/Net.js Resolved
Comment thread src/Node/Net.js Resolved
Comment thread src/Node/Net.purs Resolved
Comment thread src/Node/Net.purs Resolved
Comment thread src/Node/Net.purs Resolved
joneshf added 7 commits May 11, 2019 08:23
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.
@joneshf

joneshf commented May 11, 2019

Copy link
Copy Markdown
Collaborator Author

I think I hit all the comments. Mind giving it another look over?

Also, any idea how to get CI working?

@joneshf
joneshf requested a review from hdgarrood May 11, 2019 19:23
@hdgarrood

Copy link
Copy Markdown
Member

I think CI will "just work" as soon as there's a .travis.yml file in master.

@hdgarrood

Copy link
Copy Markdown
Member

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.
@garyb

garyb commented May 13, 2019 •

Copy link
Copy Markdown
Member

I've added a GITHUB_TOKEN to the travis env so it should auto publish to pursuit when a GH release is made too (the .travis.yml was already set up for that).

@joneshf

joneshf commented May 19, 2019

Copy link
Copy Markdown
Collaborator Author

@hdgarrood Bumping in case this dropped off your plate.

@hdgarrood hdgarrood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great, thanks! I'd say this is ready for an initial release.

Comment thread bower.json Resolved
Per review, this syntax should be a bit clearer.
@joneshf
joneshf merged commit 1fd91c8 into master May 26, 2019
@joneshf
joneshf deleted the joneshf/initial-work branch May 26, 2019 23:21
@joneshf

joneshf commented May 26, 2019

Copy link
Copy Markdown
Collaborator Author

Thanks for the review!

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