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

Support for Node 10.x & forces ASCII for socket writes #23

Merged
merged 3 commits into from
Jul 18, 2018

Conversation

alecgorge
Copy link
Contributor

Closes #22.

The tests still don't pass but it gives me an auth error with the test API key and when I use my own API key the test times out even though with DEBUG=* I can see everything worked fine.

@alecgorge
Copy link
Contributor Author

alecgorge commented Jul 16, 2018

The tests were originally timing out here because node-mitm doesn't stub out net.Socket.constructor.

The issue with the tests failing on v10.x are because of failures of node-mitm as well. The stubbed out socket implementation returns undefined except for v0.10.x which returns {}. Both are meant to indicate no error. In recent version of node.js (including 10.x), there is an explicit check to make sure the underlying write functions (which node-mitm stubs out) only return 0 to indicate no error through a err !== 0 check.

I don't want to submit those patches to node-mitm because:

  1. I've already sunk way more time than I wanted into this to add some simple metrics to my app
  2. It already should work but the test is broken

Can we have this merged and a new release cut to support 10.x and when you have time go through to make all the tests work?


The moral of the story is that the error message is (almost) always exactly right haha:

The "err" argument must be of type number. Received type undefined

@janxious
Copy link
Contributor

Probably can. One of us will look into this more tomorrow and probs cut a new release. Thanks for looking into it!

@alecgorge
Copy link
Contributor Author

Thanks so much!

@janxious
Copy link
Contributor

@alecgorge I continue to be a node novice, so do you have an opinion on the best thing to do with changes to package-lock.json in dependencies? commit whatever npm spits out?

I have a changeset that fixes the tests, and your links helped me figure out how to do it.

@janxious janxious merged commit 8a4e82e into Instrumental:master Jul 18, 2018
@alecgorge
Copy link
Contributor Author

@janxious yup, always commit whatever npm spits it

@janxious
Copy link
Contributor

Cool.

Also, there is a new version 2.3.0 out for ya.

@moll
Copy link

moll commented Sep 13, 2018

Just FYI I've fixed Mitm.js for Node v10. It's waiting on confirmation (tracked in moll/node-mitm#48) and should be out as patch-level release soon.

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