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

Result.Ok throws when you put a kafka-node client in it #167

Open
JesterXL opened this issue Dec 3, 2017 · 5 comments
Open

Result.Ok throws when you put a kafka-node client in it #167

JesterXL opened this issue Dec 3, 2017 · 5 comments

Comments

@JesterXL
Copy link

JesterXL commented Dec 3, 2017

When I put a Kafka client inside of a Result.Ok, it fails to console.log and throws an exception from debug-representation.

Steps to reproduce

npm i kafka-node folktale
Then:

const kafka = require('kafka-node');
const Result = require('folktale/result');
a = new kafka.Client('some url');
try {
    that = Result.Ok(a);
    // here's where it crashes
    console.log(that);
} catch(e) {
    console.error(e);
}

Expected behaviour

Not throw.

Observed behaviour

It throws an error.

Environment

(Describe the environment where the problem happens. This usually includes:

  • OS: Mac OSX 10.12.6
  • JavaScript VM: Node v8.1.2
  • Folktale version: 2.0.1
    )

Additional information

(Anything else you feel relevant to the issue)

@JesterXL
Copy link
Author

JesterXL commented Dec 3, 2017

Further investigation reveals it could be the Kafka client class is just really large, and you get a RangeError: Maximum call stack size exceeded if you dig into the objectToKeyValuePairs in debug-representation.js. Still debugging...

@JesterXL
Copy link
Author

JesterXL commented Dec 3, 2017

Heh, a quick hack/workaround is to provide your own toStringfunction:

that = Result.Ok({a: a, toString: () => 'sup'})

@robotlolita
Copy link
Member

It could be that the Kafka client object has cycles. I don't think we handle recursive objects in the representation (and we should!).

But also, we should allow defining a depth for larger objects there too...

@JesterXL
Copy link
Author

JesterXL commented Dec 3, 2017

Still reading the code, so bear with me; for Node could we fall back to inspect which has depth, else have a custom browser one that handles depth? I haven't really looked at Object inspectors for the browser in a long while so not sure what options we have there.

@robotlolita
Copy link
Member

We could, but then we'd be limited to how Node prints objects. Since Node doesn't know about Folktale's unions, the representation would look kinda like JSON.stringify's, which is less useful here.

@robotlolita robotlolita added this to the 4.0.0 milestone Jul 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants