Skip to content

Conversation

dmitshur
Copy link
Member

This is an implementation of possible solution 2, with modifications based on additional findings.

The reason I went with 2 instead of 1 as originally planned is described in #279 (comment).

See commit messages for full details.

This should make all tests pass.

…e/Date.

Fixes #279 (initial report).

The internalization/externalization code only checks if the time
package is present and assumes time.Unix and time.Time are available if
so. Previously, it was possible for time package to be present, but
either time.Unix or time.Time were not (due to dead code elimination),
causing possible panics.

In time package itself, ensure time.Unix func and time.Time struct it
returns are always included (despite DCE) to prevent that from
happening.
@dmitshur dmitshur force-pushed the fix-timeTime-Date-inexternalization-possible-solution-2 branch from f593654 to b906695 Compare August 30, 2015 08:16
Fixes #279 (additional findings).

When time package is not imported, it's still possible to internalize a
Date object (into an interface{}). Due to fallthrough being default,
this resulted in Function case running when internalizing Date with no
time package.

That makes no sense, and means that trying to externalize the Date
object back into JavaScript world would fail to produce the original
Date object (also, calling methods on it would obviously fail, since
it's not a real Date object).

For example, consider the following code that doesn't import time
package, it can now run and produce expected results:

	date := js.Global.Get("Date").New().Interface()
	js.Global.Get("console").Call("log", date)
@dmitshur dmitshur force-pushed the fix-timeTime-Date-inexternalization-possible-solution-2 branch from b906695 to dfffae7 Compare August 30, 2015 08:27
@neelance
Copy link
Member

LGTM. I would however prefer to negate the expression to if (timePkg === undefined) { and not use an else clause. That would highlight the fact that the normal execution path is to return a time.Time and the if clause catches the special case that the time package is not available.

@dmitshur
Copy link
Member Author

That sounds like a good idea, I will make that change. Thanks!

Apply feedback from
#286 (comment).
@dmitshur dmitshur merged commit d0cc457 into fix-timeTime-Date-inexternalization Aug 30, 2015
@dmitshur dmitshur deleted the fix-timeTime-Date-inexternalization-possible-solution-2 branch August 30, 2015 20:41
dmitshur added a commit that referenced this pull request Aug 30, 2015
Apply feedback from
#286 (comment).
@dmitshur dmitshur mentioned this pull request Aug 31, 2015
dmitshur added a commit that referenced this pull request Aug 31, 2015
Apply feedback from
#286 (comment).
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.

2 participants