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

Use util.inspect with depth: null for traceAny on node.js #5

Merged
merged 1 commit into from
May 4, 2016
Merged

Use util.inspect with depth: null for traceAny on node.js #5

merged 1 commit into from
May 4, 2016

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented May 4, 2016

Because currently recursive types without a Show instance are pretty much non-printable.

@jplatte
Copy link
Contributor Author

jplatte commented May 4, 2016

It seems like this way of checking whether we're running on node.js doesn't work (with node v0.10). I'll see if I can find something better. @hdgarrood did you ever have this problem with purescript-affjax?

@garyb
Copy link
Owner

garyb commented May 4, 2016

Maybe we make it simpler and just check if util and util.inspect are in scope, and if so, use them? This is only for debugging so doesn't have to be super correct in its implementation anyway. Perhaps worth enabling the colors option too?

@hdgarrood
Copy link

I don't think we should worry about node 0.10 if it makes things significantly harder; we don't with the purescript-node libraries. I think we should support Node 4 but not older versions.

@jplatte
Copy link
Contributor Author

jplatte commented May 4, 2016

Okay, I've rewritten the commit so it checks for util.inspect (and enables color output). Let's see if it works like this...

@jplatte
Copy link
Contributor Author

jplatte commented May 4, 2016

Doesn't work like this. Seems like you have to import util first with node 0.10. I'll try !== undefined now as well, because I'm not sure if typeof is meant to work on undefined symbols (it obviously doesn't with node 0.10).

@hdgarrood
Copy link

You also have to import util on node 6, it seems. Maybe it's just in the repl that the node api modules like fs and util and so on get automatically required.

@jplatte
Copy link
Contributor Author

jplatte commented May 4, 2016

Oh, okay. And I guess I'll also have to use module.require, not just require? The latter is what I found in the docs, but of course it doesn't work ^^°

@hdgarrood
Copy link

Also, since we're here, maybe we should consider providing a version of traceAny that lets you specify the depth you want?

You shouldn't need module.require, just require should work. How is it failing?

@jplatte
Copy link
Contributor Author

jplatte commented May 4, 2016

@jplatte
Copy link
Contributor Author

jplatte commented May 4, 2016

Okay, now I'm really out of ideas. It just keeps saying that everything I check for being undefined is undefined, and that is somehow is an error. What's going on here? :/

@hdgarrood
Copy link

Ah, that's the linter. You have to tell the linter separately that a global require exists, by putting a comment at the top of the file, like this: https://github.com/purescript-node/purescript-node-streams/blob/master/src/Node/Stream.js#L1-L2 (well, replace exports or Buffer with require, but yeah)

@jplatte
Copy link
Contributor Author

jplatte commented May 4, 2016

Thanks, should work now then. Actually I got the same error locally from my linter editor plugin, I just didn't expect that the server reported linter errors as build fails.

Because currently recursive types without a `Show` instance are pretty much non-printable.
@jplatte
Copy link
Contributor Author

jplatte commented May 4, 2016

Aaand another rewrite... This time the linter didn't like my code style (but only on the server, no local errors) ^^°

@jplatte
Copy link
Contributor Author

jplatte commented May 4, 2016

Finally! :)

@garyb
Copy link
Owner

garyb commented May 4, 2016

Thanks, sorry this turned out so painful! 😄

@garyb garyb merged commit 7752b8d into garyb:master May 4, 2016
@jplatte jplatte deleted the patch-1 branch May 4, 2016 12:42
@jplatte
Copy link
Contributor Author

jplatte commented May 4, 2016

No problem, at least I learned something :)

@jplatte
Copy link
Contributor Author

jplatte commented May 4, 2016

Also, since we're here, maybe we should consider providing a version of traceAny that lets you specify the depth you want?

That might be useful, but I think it should be in a seperate package that wraps the whole util module.

@garyb
Copy link
Owner

garyb commented May 4, 2016

^ agreed. The depth argument would only apply on node too, as when you console.log in the browser you get the interactive expandable thing rather than a string value appearing in the log.

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