/core

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 runtime error in native toString (handling null values) #183

Closed
wants to merge 2 commits into
base: master
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@ThomasWeiser @kasbah @evancz @eeue56 @shamrin
@ThomasWeiser
Contributor

ThomasWeiser commented Feb 24, 2015

toString needs special case for null value, otherwise it throws a type error.

To reproduce the bug: toString Json.Encode.null
TypeError: Cannot use 'in' operator to search for '' in null_

@ThomasWeiser ThomasWeiser referenced this pull request Feb 24, 2015

Closed

Fix runtime error in native toString #182

@ThomasWeiser

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser Mar 10, 2015

Contributor

First I made the PR #182 against the stable branch. You wrote a comment
that made me think I should make the PR against the master branch. So I
did (#183, still open) and closed the first one. Should I reopen #182?

Thanks!

On 10.03.2015 13:19, Kaspar Emanuel wrote:

Why was this closed?

—
Reply to this email directly or view it on GitHub
https://github.com/elm-lang/core/pull/183#issuecomment-78042547.
Contributor

ThomasWeiser commented Mar 10, 2015

First I made the PR #182 against the stable branch. You wrote a comment
that made me think I should make the PR against the master branch. So I
did (#183, still open) and closed the first one. Should I reopen #182?

Thanks!

On 10.03.2015 13:19, Kaspar Emanuel wrote:

Why was this closed?

—
Reply to this email directly or view it on GitHub
https://github.com/elm-lang/core/pull/183#issuecomment-78042547.
@kasbah

This comment has been minimized.

Show comment
Hide comment
@kasbah

kasbah Mar 10, 2015

Contributor

Yes, no it's fine. I deleted my comment a second after I made it actually. I erroneously thought this PR was closed without comment.
Contributor

kasbah commented Mar 10, 2015

Yes, no it's fine. I deleted my comment a second after I made it actually. I erroneously thought this PR was closed without comment.
@kasbah

This comment has been minimized.

Show comment
Hide comment
@kasbah

kasbah Mar 10, 2015

Contributor

Though you probably want to rebase this on current master so it can be merged more easily.
Contributor

kasbah commented Mar 10, 2015

Though you probably want to rebase this on current master so it can be merged more easily.
@ThomasWeiser

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser Mar 10, 2015

Contributor

Ok, I rebased the commit onto current master
Contributor

ThomasWeiser commented Mar 10, 2015

Ok, I rebased the commit onto current master
@ThomasWeiser

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser Apr 18, 2015

Contributor

@evancz: Would be nice to have the fix in the upcoming 0.15 release.
Contributor

ThomasWeiser commented Apr 18, 2015

@evancz: Would be nice to have the fix in the upcoming 0.15 release.
@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Apr 19, 2015

Member

I don't think this is really the right fix. I would expect a Json.Value to appear as an <internal value> or something. I don't think raw JS objects should be printed out the same as records or whatever. I'm not sure how to do this at the moment though.
Member

evancz commented Apr 19, 2015

I don't think this is really the right fix. I would expect a Json.Value to appear as an <internal value> or something. I don't think raw JS objects should be printed out the same as records or whatever. I'm not sure how to do this at the moment though.
@ThomasWeiser

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser Apr 20, 2015

Contributor

I am not sure, whether Json.Value should be printable or not. I find it very useful to be able to Debug.log these values. Otherwise one would have to patch the generated JavaScript code to insert the logging, which is much more awkward.

Anyway, the current code for toString is incomplete in the sense, that it throws a confusing runtime error for the JavaScript null value. IMHO this should be fixed anyway.
Contributor

ThomasWeiser commented Apr 20, 2015

I am not sure, whether Json.Value should be printable or not. I find it very useful to be able to Debug.log these values. Otherwise one would have to patch the generated JavaScript code to insert the logging, which is much more awkward.

Anyway, the current code for toString is incomplete in the sense, that it throws a confusing runtime error for the JavaScript null value. IMHO this should be fixed anyway.
@ThomasWeiser

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser May 25, 2015

Contributor

I don't think this is really the right fix. I would expect a Json.Value to appear as an <internal value> or something.

Ok, I have modified the the code to return "<internal structure>" for null values, as @evancz proposed.
Contributor

ThomasWeiser commented May 25, 2015

I don't think this is really the right fix. I would expect a Json.Value to appear as an <internal value> or something.

Ok, I have modified the the code to return "<internal structure>" for null values, as @evancz proposed.

@ThomasWeiser ThomasWeiser referenced this pull request Jul 14, 2015

Closed

List of known runtime errors #913

1 of 7 tasks complete

@rtfeldman rtfeldman referenced this pull request Aug 29, 2015

Open

potential runtime exceptions #377

@ThomasWeiser

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser Oct 11, 2015

Contributor

Please consider fixing for the upcoming 3.0.0 release (I've just rebased the PR).

This unnecessary runtime error did bite me multiple times.
Contributor

ThomasWeiser commented Oct 11, 2015

Please consider fixing for the upcoming 3.0.0 release (I've just rebased the PR).

This unnecessary runtime error did bite me multiple times.

@jvoigtlaender jvoigtlaender referenced this pull request Jan 19, 2016

Closed

Allow toString to handle null values #477

@jvoigtlaender jvoigtlaender changed the title from Fix runtime error in native toString to Fix runtime error in native toString (handling null values) Jan 19, 2016

Show outdated Hide outdated src/Native/Utils.js
if (v === null)
{
return '<internal structure>';
}

This comment has been minimized.

Show comment
Hide comment
@shamrin

shamrin Jan 19, 2016

The block above seems to have wrong indentation. Tabs vs spaces?

In addition, should this code also handle undefined? E.g. with == null.
Copy URL
@shamrin

shamrin Jan 19, 2016

The block above seems to have wrong indentation. Tabs vs spaces?

In addition, should this code also handle undefined? E.g. with == null.

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser Jan 19, 2016

Contributor

As far as I can see the source and my fix are both tab indented.

undefined is handled well with the existing code, so it seems sufficient just to test for null.
Copy URL
@ThomasWeiser

ThomasWeiser Jan 19, 2016

Contributor

As far as I can see the source and my fix are both tab indented.

undefined is handled well with the existing code, so it seems sufficient just to test for null.

This comment has been minimized.

Show comment
Hide comment
@shamrin

shamrin Jan 19, 2016

You are right, there are only tabs in the code. However, you still have unnecessary indent for if body. Brace { should be below i.
Copy URL
@shamrin

shamrin Jan 19, 2016

You are right, there are only tabs in the code. However, you still have unnecessary indent for if body. Brace { should be below i.

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser Jan 19, 2016

Contributor

ooops
Copy URL
@ThomasWeiser

ThomasWeiser Jan 19, 2016

Contributor

ooops
@ThomasWeiser
Fix runtime error in native toString 
`toString` needs special case for `null` value, otherwise it throws a type error.
f3c38bb
@ThomasWeiser

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser Jan 19, 2016

Contributor

Rebased again on current master
Contributor

ThomasWeiser commented Jan 19, 2016

Rebased again on current master

@jvoigtlaender jvoigtlaender referenced this pull request Jan 19, 2016

Closed

Basics.toString leads to runtime error on Html values #488

@jvoigtlaender jvoigtlaender referenced this pull request Jan 28, 2016

Closed

Runtime Error: Cannot use 'in' operator to search for 'ctor' in null #438

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 11, 2016

Contributor

FWIW it is very useful to be able to log Json.Value and see the actual JS object. E.g, when working with Http requests, it's a lot nicer if I can debug the data coming down right in my Elm code where it comes down, rather than make a port which I have to send stuff to in order to debug.
Contributor

eeue56 commented Mar 11, 2016

FWIW it is very useful to be able to log Json.Value and see the actual JS object. E.g, when working with Http requests, it's a lot nicer if I can debug the data coming down right in my Elm code where it comes down, rather than make a port which I have to send stuff to in order to debug.

@jvoigtlaender jvoigtlaender referenced this pull request Apr 17, 2016

Closed

toString isn't type safe for Html #560

@evancz

@evancz evancz closed this Jun 26, 2016

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