Skip to content

Date to time.time causes Uncaught Error: cannot internalize time.Time #287

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

Closed
Tobiasartz opened this issue Aug 31, 2015 · 11 comments · Fixed by #288
Closed

Date to time.time causes Uncaught Error: cannot internalize time.Time #287

Tobiasartz opened this issue Aug 31, 2015 · 11 comments · Fixed by #288

Comments

@Tobiasartz
Copy link

According to the docs (if I'm reading them correctly) I can expose a method to Javascript and call it using MakeWrapper. GopherJS will take care of the conversion for the arguments within the exposed methods. (http://godoc.org/github.com/gopherjs/gopherjs/js#MakeWrapper)

However I can't seem to get a date object converted in to time.Time

package main

import "github.com/gopherjs/gopherjs/js"
import "time"

func main() {
    js.Global.Set("pet", map[string]interface{}{
        "New": New,
    })
}

type Pet struct {
    birthday time.Time
}

func New(date time.Time) *js.Object {
    return js.MakeWrapper(&Pet{date})
}

func (p *Pet) Birthday() time.Time {
    return p.birthday
}

func (p *Pet) SetBirthday(date time.Time) {
    p.birthday = date
}

Playground link: http://www.gopherjs.org/playground/#/JEcteZCXUP

When I call dog.New(new Date()) I get
Uncaught Error: cannot internalize time.Time

Am I missing something here?

@dmitshur
Copy link
Member

This looks like a bug in $externalizeFunction. Here's a simpler reproduce:

package main

import (
    "fmt"
    "time"

    "github.com/gopherjs/gopherjs/js"
)

func Foo(x string) {
    fmt.Println(x)
}

func Bar(x []int) {
    fmt.Println(x)
}

func Baz(x time.Time) {
    fmt.Println(x)
}

func main() {
    js.Global.Set("Foo", Foo)
    js.Global.Set("Bar", Bar)
    js.Global.Set("Baz", Baz)
}
<script type="text/javascript">
    Foo("hello");

    var x = new Int32Array(3);
    x[0] = 1;
    x[1] = 2;
    x[2] = 3;
    Bar(x);

    Baz(new Date());
</script>

Output:

image

I've tried a few various types, and it seems to only have a problem with time.Time/Date.

I've confirmed this is not related to #279 (because adding var _ = time.Unix(0, 0) does not help, whereas it does with issue #279).


Looking at the generated JavaScript for the above sample, it looks odd. I'd expect to see $Date rather than time.Time in that funcType$2:

sliceType = $sliceType($emptyInterface);
funcType = $funcType([$String], [], false);
sliceType$1 = $sliceType($Int);
funcType$1 = $funcType([sliceType$1], [], false);
funcType$2 = $funcType([time.Time], [], false);
...
main = function() {
    var $ptr;
    $global.Foo = $externalize(Foo, funcType);
    $global.Bar = $externalize(Bar, funcType$1);
    $global.Baz = $externalize(Baz, funcType$2);
};

But that might be intended and the problem could be elsewhere.

Edit: Yeah, that part might be fine. The problem might be in $internalize itself. It's definitely in one of these related funcs.

@dmitshur
Copy link
Member

It seems the following patch for $internalize fixes the problem. It catches the case when the internalize object is a Date and it's being internalized into a Go type time.Time struct, rather than into an interface.

I'm not sure if this is the right solution/approach, but it seems to work, and it seems symmetrical ($externalize also has a special case for Date/time.Time in its case $kindStruct section). @neelance, does it look right to you, or is it intentional for $internalize not to have this already?

diff --git a/compiler/prelude/jsmapping.go b/compiler/prelude/jsmapping.go
index 38874da..8566a20 100644
--- a/compiler/prelude/jsmapping.go
+++ b/compiler/prelude/jsmapping.go
@@ -313,6 +313,13 @@ var $internalize = function(v, t, recv) {
     }
     return s;
   case $kindStruct:
+    if (v !== null && v !== undefined && v.constructor === Date) {
+      var timePkg = $packages["time"];
+      if (timePkg !== undefined && t === timePkg.Time) {
+        return timePkg.Unix(new $Int64(0, 0), new $Int64(0, v.getTime() * 1000000));
+      }
+    }
+
     var noJsObject = {};
     var searchJsObject = function(t) {
       if (t === $jsObjectPtr) {

Here's the output of my sample program after applying this patch:

image

@neelance
Copy link
Member

Yes, it is right that we should handle this case. My opinion:

  1. We should check first for t === timePkg.Time and if it holds we should expect a Date and panic otherwise (maybe except null and undefined which we could map to the zero Time).
  2. When we have this, we should merge it with the interface{} case by making that one call $internalize with t set to timePkg.Time.

@dmitshur
Copy link
Member

That sounds good, I'll make those changes in the PR.

@dmitshur
Copy link
Member

maybe except null and undefined which we could map to the zero Time

Can you elaborate on this? How/why would null and undefined map to zero time.Time? It's not a pointer type.

@neelance
Copy link
Member

I was just thinking about the timestamp 0. Maybe it would be better to also panic in these cases and make sure that you can use *time.Time if necessary? It's just that JS is really not type-safe, so you often have the case where some timestamp field just does not exist. We should give some option to handle it nicely. E.g. encoding/json fails if you unmarshal null into a time.Time, but it works with *time.Time.

dmitshur added a commit that referenced this issue Aug 31, 2015
@dmitshur
Copy link
Member

dmitshur commented Sep 1, 2015

E.g. encoding/json fails if you unmarshal null into a time.Time, but it works with *time.Time.

I've looked into that, and it's not quite true. There's nothing fundamentally different about unmarshaling into a time.Time/*time.Time and into string/*string.

The type time.Time implements json.Unmarshaler type. So an entry like "2015-09-01T07:33:08.862Z" can be unmarshaled into time.Time, but null would error:

parsing time "null" as ""2006-01-02T15:04:05Z07:00"": cannot parse "null" as """

Unmarshalling null into any pointer type results that pointer having zero value. So if you unmarshal null into *time.Time, you get a (*time.Time)(nil), which is not the same as a zero time:

t := (*time.Time)(nil)
fmt.Println(t.IsZero())

// panic: runtime error: invalid memory address or nil pointer dereference

Can you please elaborate on what you mean by "timestamp 0"? When does it occur in JavaScript and how is it normally used? In what situation would you want to try to internalize a null into a *time.Time and expect a non-nil result that is equal to zero time?

dmitshur added a commit that referenced this issue Sep 1, 2015
Attempt to follow suggestion in
#287 (comment).

Not quite sure it's an improvement at this time, but maybe it just
needs more work?

No handling of null or undefined values instead of Date. Also no
support for *time.Time at this time.
@dmitshur
Copy link
Member

dmitshur commented Sep 1, 2015

Yes, it is right that we should handle this case. My opinion:

  1. We should check first for t === timePkg.Time and if it holds we should expect a Date and panic otherwise (maybe except null and undefined which we could map to the zero Time).
  2. When we have this, we should merge it with the interface{} case by making that one call $internalize with t set to timePkg.Time.

This sounded like a good idea when I first heard it, but I tried it now and I'm not very happy with how it's working out so far. Maybe you'll have ideas on how to improve it, or maybe you'll agree it's not an improvement. Well, I'm still unsure...

#293 - Please take a look and review.

For one, I don't like that the var timePkg = $packages["time"]; if (timePkg !== undefined) { ... checks are being done ad-hoc in multiple places, more often than before. As far as I can tell, all packages will be loaded before the program begins execution, and no new packages are ever loaded. So doesn't it make more sense to calculate if time package is available just once, rather than every time? Or is it too insignificant in performance?

Also, the if (timePkg === undefined) { special handling still has to stay in case $kindInterface as before, which seems to detract from the refactor's effectiveness.

@neelance
Copy link
Member

neelance commented Sep 1, 2015

I would not worry about performance, this is really insignificant. You can remove line 280. Then it looks quite good to me.

@dmitshur
Copy link
Member

dmitshur commented Sep 1, 2015

Nice idea, done in 81cc5d8.

dmitshur added a commit that referenced this issue Sep 2, 2015
Refactor based on suggestion at
#287 (comment).

Panic on null or undefined values instead of Date, no support for
*time.Time.

Remove unneeded declaration. The same variable is already declared in
parent scope.
@dmitshur
Copy link
Member

dmitshur commented Sep 2, 2015

I would not worry about performance, this is really insignificant. You can remove line 280. Then it looks quite good to me.

All right, I've cleaned it up and added that refactor commit to #288.

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 a pull request may close this issue.

3 participants