Skip to content
This repository was archived by the owner on Apr 30, 2019. It is now read-only.

Fix js.ValueOf() with map[string]interface{} under gopherjs - fixes #19 #20

Merged
merged 2 commits into from
Oct 11, 2018
Merged

Fix js.ValueOf() with map[string]interface{} under gopherjs - fixes #19 #20

merged 2 commits into from
Oct 11, 2018

Conversation

ncw
Copy link
Contributor

@ncw ncw commented Oct 9, 2018

I'm not 100% sure this is the right fix, but it seems to work!

hajimehoshi
hajimehoshi reviewed Oct 9, 2018
View reviewed changes
Copy link
Member

@hajimehoshi hajimehoshi left a comment

Choose a reason for hiding this comment

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

Thank you!

js/js_notwasm.go Outdated
@@ -181,7 +181,7 @@ func ValueOf(x interface{}) Value {
return x.Value
case nil:
return Null()
case bool, int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float32, float64, unsafe.Pointer, string:
case bool, int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float32, float64, unsafe.Pointer, string, map[string]interface{}:
Copy link
Member

Choose a reason for hiding this comment

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

and []interface{}?

https://golang.org/pkg/syscall/js/?GOOS=js&GOARCH=wasm#ValueOf

Copy link
Member

Choose a reason for hiding this comment

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

And, please add tests.

@ncw
Copy link
Contributor Author

ncw commented Oct 10, 2018

Will add []interface{} and tests shortly!

@ncw
Copy link
Contributor Author

ncw commented Oct 11, 2018

I've updated the commit with tests too!

I've added a commit to fix a typo in the .travis.yml also which meant it was testing gopherjs not gopherwasm.

I'm still not 100% sure travis is testing the pull request though rather than master.

In .travis.yml there is the line

  - go get github.com/gopherjs/gopherwasm@master

Which seems to me it overwrites the already checked out pull request with master doesn't it? Or maybe it doesn't since this is outside GOPATH? 😕

@hajimehoshi
Copy link
Member

Thank you for fixing! I didn't realize the misspelling :-)

Which seems to me it overwrites the already checked out pull request with master doesn't it?

This go get is module-get for the current directory and doesn't affect $GOPATH/go. GopherJS doesn't recognize modules, so the test includes both go get. After GopherJS accepts modules, I'll merge them into module-get.

hajimehoshi
hajimehoshi reviewed Oct 11, 2018
View reviewed changes
@ncw
Fix js.ValueOf() with map[string]interface{} and []interface{} under …
1a85228 
…gopherjs

This means that gopherjs ValueOf supports the same types as supported
by the wasm version.

Add a test for js.ValueOf also.

Fixes gopherjs#19
@ncw
Copy link
Contributor Author

ncw commented Oct 11, 2018

I've added JSON.stringify - it works much better :-)

I had to only have one item in the object as the order of the keys can vary, but I don't think that is a problem.

Thanks for your explanation about the go mod stuff.

hajimehoshi
hajimehoshi approved these changes Oct 11, 2018
View reviewed changes
Copy link
Member

@hajimehoshi hajimehoshi left a comment

Choose a reason for hiding this comment

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

Thank you, lgtm!

@hajimehoshi
Copy link
Member

I had to only have one item in the object as the order of the keys can vary, but I don't think that is a problem.

Right, that's a good point. I think the current test is totally fine.

@hajimehoshi hajimehoshi merged commit b0e1786 into gopherjs:master Oct 11, 2018
@ncw ncw deleted the fix-19 branch October 11, 2018 20:29
@ncw
Copy link
Contributor Author

ncw commented Oct 11, 2018

PS Thanks for a great project - it is very useful! I'm using it here https://gpython.org/

@hajimehoshi
Copy link
Member

Thank you for your PR! Is that a Python interpreter in Go? Awesome work!

@ncw
Copy link
Contributor Author

ncw commented Oct 12, 2018

Thank you for your PR! Is that a Python interpreter in Go? Awesome work!

It is! Not the full thing alas since too many modules are in C but the core is there :-) I was amazed that gopherjs just compiled it no fuss and it worked! (Well I found one bug in gopherjs (which I reported) but that is pretty minor! Try 2**10 in the gpython repl if you want to see it in action!)

@hajimehoshi
Copy link
Member

hajimehoshi commented Oct 12, 2018 •
edited
Loading

Cool! I tried but on GopherJS version, there were errors:

>>> 2 ** 10
[USER]: runtime error: invalid memory address or nil pointer dereference
Error: runtime error: invalid memory address or nil pointer dereference
    at $callDeferred (https://gpython.org/gpython.js:4:22511)
    at $panic (https://gpython.org/gpython.js:4:22957)
    at AK (https://gpython.org/gpython.js:10:2423)
    at Object.$throwNilPointerError (https://gpython.org/gpython.js:4:486)
    at Object.get (https://gpython.org/gpython.js:4:4522)
    at Object.$packages.math/big.BL.ptr.Exp (https://gpython.org/gpython.js:38:95689)
    at Object.$packages.github.com/go-python/gpython/py.BH.ptr.pow (https://gpython.org/gpython.js:41:102703)
    at Object.$packages.github.com/go-python/gpython/py.BH.ptr.M__pow__ (https://gpython.org/gpython.js:41:103772)
    at $packages.github.com/go-python/gpython/py.EQ.M__pow__ (https://gpython.org/gpython.js:41:236522)
    at Object.AZ [as Pow] (https://gpython.org/gpython.js:41:73603)
    at V (https://gpython.org/gpython.js:44:18787)
    at DO (https://gpython.org/gpython.js:44:91592)
    at DU (https://gpython.org/gpython.js:44:107450)
    at Object.DW [as Run] (https://gpython.org/gpython.js:44:109015)
    at Object.$packages.github.com/go-python/gpython/repl.G.ptr.Run (https://gpython.org/gpython.js:56:3575)
    at $b (https://gpython.org/gpython.js:60:3908)
    at $b (https://gpython.org/gpython.js:59:2625)
    at r (https://gpython.org/gpython.js:4:23443)
    at $runScheduled (https://gpython.org/gpython.js:4:24007)
    at $schedule (https://gpython.org/gpython.js:4:24184)
    at $go (https://gpython.org/gpython.js:4:23907)
    at https://gpython.org/gpython.js:59:2237
    at jQuery.fn.init.e.$externalizeWrapper.e.$externalizeWrapper (https://gpython.org/gpython.js:4:28925)
    at a (https://cdnjs.cloudflare.com/ajax/libs/jquery.terminal/1.23.2/js/jquery.terminal.min.js:40:82615)
    at jQuery.fn.init.k (https://cdnjs.cloudflare.com/ajax/libs/jquery.terminal/1.23.2/js/jquery.terminal.min.js:40:83463)
    at Object.ENTER (https://cdnjs.cloudflare.com/ajax/libs/jquery.terminal/1.23.2/js/jquery.terminal.min.js:40:14915)
    at HTMLHtmlElement.$e (https://cdnjs.cloudflare.com/ajax/libs/jquery.terminal/1.23.2/js/jquery.terminal.min.js:40:28362)
    at HTMLHtmlElement.dispatch (https://code.jquery.com/jquery-latest.js:4641:9)
    at HTMLHtmlElement.elemData.handle (https://code.jquery.com/jquery-latest.js:4309:28)

EDIT: Ah ok, I understood this is the bug in GopherJS.

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

@hajimehoshi hajimehoshi hajimehoshi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ncw @hajimehoshi