Skip to content

Fix internalize case for Date into time.Time struct. #288

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 3, 2015
Merged

Fix internalize case for Date into time.Time struct. #288

merged 4 commits into from
Sep 3, 2015

Conversation

dmitshur
Copy link
Member

Don't merge, this PR is a WIP (more commits to be pushed) and in need of review. Ready for review.

Add a failing test case, then fix #287.

@dmitshur
Copy link
Member Author

As desired, commit 098b579 added a failing test case and it indeed failed with:

--- FAIL: TestInternalizeDate (0.01s)
/tmp/test.592009139:4
return;}$goroutine.exit=true;}catch(err){$goroutine.exit=true;throw err;}final
                                                                    ^
Error: cannot internalize time.Time
    at $callDeferred (/tmp/test.592009139:4:27597)
    at $panic (/tmp/test.592009139:4:28221)
    at $b (/tmp/test.592009139:27:44660)
    at $callDeferred (/tmp/test.592009139:4:27768)
    at $panic (/tmp/test.592009139:4:28221)
    at $internalize (/tmp/test.592009139:4:39702)
    at v.$externalizeWrapper (/tmp/test.592009139:4:35661)
    at eval (eval at <anonymous> (/tmp/test.592009139:28:16346), <anonymous>:1:18)
    at eval (native)
    at $packages.github.com/gopherjs/gopherjs/js_test.AE [as F] (/tmp/test.592009139:28:16346)
    at $packages.testing.BN (/tmp/test.592009139:27:44958)
    at $goroutine (/tmp/test.592009139:4:28817)
    at $runScheduled [as _onTimeout] (/tmp/test.592009139:4:29579)
    at Timer.listOnTimeout (timers.js:119:15)
FAIL    github.com/gopherjs/gopherjs/js 0.503s

The next commit will contain a fix that should get the failing test case to pass.

dmitshur
dmitshur reviewed Aug 31, 2015
View reviewed changes
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) {
Copy link
Member Author

Choose a reason for hiding this comment

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

There's probably a better way to do this instead of v !== null && v !== undefined, isn't there?

I needed it to avoid:

  • TypeError: Cannot read property 'constructor' of null
  • TypeError: Cannot read property 'constructor' of undefined

Copy link
Member

Choose a reason for hiding this comment

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

You can do just v, but that matches a few more cases. I'd go with the solution you have.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd go with the solution you have.

Sounds good.

@dmitshur
Copy link
Member Author

Commit 3844adb has fixed the failing test case.

This PR is ready for full review now.

@neelance
Copy link
Member

Please see my comment on #287.

@dmitshur
Add TestInternalizeExternalizeNull.
f2a7f63 
It's just like TestInternalizeExternalizeUndefined, but for null. It
helped me uncover a bug in internalization code during development, and
seems like a good idea to have it for better coverage.
@dmitshur
Add failing test for internalization of Date into time.Time struct.
6cc2b7b 
Test case for #287,
currently failing.
@dmitshur
Fix internalize case for Date into time.Time struct.
55f8e77 
Fixes #287.
@dmitshur dmitshur mentioned this pull request Sep 1, 2015
Merged
@dmitshur
Handle t === timePkg.Time case on its own.
fc96db6 
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 dmitshur mentioned this pull request Sep 2, 2015
Closed
@dmitshur
Copy link
Member Author

dmitshur commented Sep 3, 2015

I've resolved that. What else does this PR need to be merged?

@neelance
Copy link
Member

neelance commented Sep 3, 2015

LGTM

dmitshur added a commit that referenced this pull request Sep 3, 2015
@dmitshur
Merge pull request #288 from gopherjs/fix-287
22fe4b5 
Fix internalize case for Date into time.Time struct.
@dmitshur dmitshur merged commit 22fe4b5 into master Sep 3, 2015
@dmitshur dmitshur deleted the fix-287 branch September 3, 2015 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Date to time.time causes Uncaught Error: cannot internalize time.Time
2 participants
@dmitshur @neelance