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

Fix errors so they provide more information #53

Closed
ulfryk opened this issue Apr 19, 2016 · 6 comments · Fixed by #97
Closed

Fix errors so they provide more information #53

ulfryk opened this issue Apr 19, 2016 · 6 comments · Fixed by #97

Comments

@ulfryk
Copy link
Member

ulfryk commented Apr 19, 2016

Now when for example Some(null); is called the only information we get with thrown error is "Illegal state exception" and stack. Would be nice if it informed that it's Monet error and that it's connected with Maybe :)

@ulfryk ulfryk added this to the 0.9 Release milestone Apr 19, 2016
@cwmyers
Copy link
Collaborator

cwmyers commented Apr 19, 2016

I feel like I made a mistake with throwing an error when Some is constructed with a null. I think Some should remain ignorant of what is inside it. It's a mistake Java 8 made and perhaps we should undo this. See here for background: https://developer.atlassian.com/blog/2015/08/optional-broken/

@ulfryk
Copy link
Member Author

ulfryk commented Apr 19, 2016

Actually I (and not only) found it really useful - runtime checks are quite sane in terms of dynamic type nature of JS (in contrast to statically typed Java or Scala). I'll vote for not removing this strict check - it makes Some a great representation of non-nullable value. It makes 'monet' Maybe a great tool for handling null in JS with runtime checks. Even if it breaks a bit Functor rules…

I can ask more monet fans - @WojciechP @dumpstate - WDYT ?

@WojciechP
Copy link
Collaborator

WojciechP commented Apr 20, 2016

Hello,
I believe that all arguments based in category theory should be preceded by the category definition we want to live in. If we consider a "small but decent" category of non-null non-undefined values, then the issue is non-existent because a function that returns nulls does not belong to the category.
If on the other hand we consider the full category of available JS values and procedures, Maybe is not a Monad - but not only because of the Some(null) thing. Recall that for any m: Maybe<A>, f: A => B, g: B =>C the following expression should always be truthy:
m.bind(f).bind(g) === m.bind(t => f(t).bind(g))
The first problem is we don't have a good equality check in JS. But let's say we can work around it implementing .equals or something. Even then the law doesn't hold: consider function f(x) { throw new UnimplementedError(); } - then the expression will not be truthy, because it will throw an exception.
The purpose of functional tools is IMO making reasoning easier, and removing the need to consider nulls is a step in good direction. I'd stick to the idealized category of non-empty values and pure functions, and keep the current check. If there is anything that requires fixing about Maybe, I'll rather cut down the signature of bind so that Some(4).bind(t => t+1) is no longer valid (one should use .map instead), but that breaks compatibility, so I'm not pressing

Sorry, it got lengthy. Bottom line: the library should help the users, and I think keeping nulls outside of Monet is a good idea.

@cwmyers
Copy link
Collaborator

cwmyers commented Apr 25, 2016

Ok. I think you've both convinced me. You are right JavaScript is programming in hard mode and we should err on the side of helping out the user. I guess the fromNull method is there as a support too.

@iantanwx
Copy link

iantanwx commented Sep 13, 2016

@WojciechP I was about to start writing a PR for this behaviour until I saw this discussion, thanks for the lengthy explanation on why fromNull exists. At first I thought it was weird, since Maybe should hold any value, but what you say makes complete sense in a JS context.

Edit: That being said, I think there should be a better error message, like Maybe cannot be initialised with null. Please use fromNull if you wish to wrap a potentially null value. Happy to submit PR.

@ulfryk
Copy link
Member Author

ulfryk commented Jan 3, 2017

Fixed in #97

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants