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

Add check to determine if in browser or node #6

Closed
wants to merge 3 commits into from

Conversation

deadfoxygrandpa
Copy link

When I simplified the public API for elm-test, it caused a runtime error in the browser since the node-specific code here runs and crashes: deadfoxygrandpa/elm-test#32

I've added some runtime checks to disable those things if it's run in the browser.

@deadfoxygrandpa
Copy link
Author

Let me know what you think, though, because the other direction I could go in is making a totally separate import for ElmTest and ElmTest.Console or something like that.

var stdin = process.stdin;
var in_node = true;
} catch (reference_error) {
var in_node = false;
Copy link

Choose a reason for hiding this comment

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

(Sorry for commenting from non-contributor)

How about this way to check:

var in_node = typeof module !== 'undefined' && module.exports

Ref: http://stackoverflow.com/questions/4224606/how-to-check-whether-a-script-is-running-under-node-js

Then, maybe sneakCase is preferable. inNode

@mgold
Copy link

mgold commented Nov 20, 2015

Just ran into this and let me say I really do appreciate your timeliness! I think the API is fine, just fix the bug. I'm going to pull this down locally and report back.

@mgold
Copy link

mgold commented Nov 20, 2015

Confirming that it works.

@deadfoxygrandpa
Copy link
Author

Hi @laszlopandy, can you get a chance to review this soon? I can change it to the solution from @igrep if you think that's better.

@igrep
Copy link

igrep commented Nov 24, 2015

👍

@mgold
Copy link

mgold commented Nov 24, 2015

Also asking for a quick merge, as this is holding up a lot of testing and dependent packages.

@laszlopandy
Copy link
Owner

Sorry guys, I didn't see this until the mention. I will merge, I just have one concern.

@deadfoxygrandpa
Copy link
Author

What's your concern?

// trigger the initial IO requests
sendResponseString(null);
break;
}
Copy link
Owner

Choose a reason for hiding this comment

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

@deadfoxygrandpa my concern is here. This fails silently if you are trying to write something to the console. It does not need to be wrapped with isNode because unlike at the top of the file, this part doesn't run by default.

@laszlopandy
Copy link
Owner

I fixed it in a different way (without wrapping everything in isNode).
19e686f

I published it, but I have only tested it with 0.15.1. Please let me know if there are any problems with 0.16.0:
http://package.elm-lang.org/packages/laszlopandy/elm-console/1.0.3/

@deadfoxygrandpa
Copy link
Author

OK, thanks. I'll test it out in browser right now.

@deadfoxygrandpa
Copy link
Author

Yep, this works with Elm 0.16.0 in browser. Thanks!

@mgold
Copy link

mgold commented Nov 24, 2015

Thanks Laszlo!

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.

4 participants