Add syscall/js #908

hajimehoshi · 2019-04-06T10:47:06Z

This PR adds a new package syscall/js. Originally syscall/js is for WebAssembly, but in GopherJS this package works as a wrapper of github.com/gopherjs/gopherjs/js. With this package, developers can use syscall/js both for GopherJS and WebAssembly.

The implementation is basically same as github.com/gohperjs/gopherwasm.

Fixes #899

hajimehoshi · 2019-04-06T11:18:45Z

--- FAIL: TestSleep (0.10s)
    test.148654449:38: Sleep(100ms) slept for only 99ms

Meh, this sounds like a flaky issue

neelance · 2019-04-06T13:32:11Z

@hajimehoshi While working on WebAssembly I've noticed that setTimeout sometimes invokes the callback 1ms early. Maybe this is the same issue. It would probably be okay to always add 1ms to the delay.

hajimehoshi · 2019-04-06T14:18:53Z

@neelance Thank. I think I'll do this in another PR later.

dmitshur

The changes in this PR look good to me.

See inline comments. Hopefully the flaky test can be worked around so the tests pass before this is merged.

tests/syscalljs_test.go

compiler/natives/fs_vfsdata.go

hajimehoshi · 2019-04-11T01:08:19Z

@neelance Hi, are you happy with this PR? Thanks!

hajimehoshi · 2019-04-17T05:02:58Z

ping @neelance

neelance · 2019-04-17T18:38:34Z

Is this passing the normal tests of the syscall/js package?https://github.com/golang/go/blob/master/src/syscall/js/js_test.go

Is there any particular need for the custom test cases of this PR?

hajimehoshi · 2019-04-18T01:48:37Z

To test js_test.go, we'd need to copy the entire file except for // +build js,wasm. Should we do that?

hajimehoshi · 2019-04-18T01:49:45Z

BTW, I put a test file at compiler/natives/src/syscall/js, but the test was not executed, I'm not sure the reason though.

neelance · 2019-04-23T10:49:59Z

Shouldn't it be possible to modify importWithSrcDir to manually include js_test.go in the build?

hajimehoshi · 2019-04-24T16:23:15Z

Almost done! I found a problem in js_test.go:

func TestIntConversion(t *testing.T) {                                                                                                                                                                                                    
        testIntConversion(t, 0)                                                                                                                                                                                                           
        testIntConversion(t, 1)                                                                                                                                                                                                           
        testIntConversion(t, -1)                                                                                                                                                                                                          
        testIntConversion(t, 1<<20)                                                                                                                                                                                                       
        testIntConversion(t, -1<<20)                                                                                                                                                                                                      
        testIntConversion(t, 1<<40)                                                                                                                                                                                                       
        testIntConversion(t, -1<<40)                                                                                                                                                                                                      
        testIntConversion(t, 1<<60)                                                                                                                                                                                                       
        testIntConversion(t, -1<<60)                                                                                                                                                                                                      
}

This cannot be compiled on 32bit-integer environment and GopherJS is an environment with 32bit integers. Replacing the test function in a usual GopherJS way didn't work. Probably the const check happens before the replacing. Any suggestions?

EDIT:

Looks like CircleCI tests passed. No worries!

On my local machine, go generate ./... && go run . test syscall/js with overflow errors, but I might miss something.

EDIT2:

My bad, I didn't add syscall/js for tests explicitly. Now CircleCI says the same error as mine:

../../../../../../../usr/local/go/src/syscall/js/js_test.go:106:23: 1 << 40 (untyped int constant 1099511627776) overflows int
../../../../../../../usr/local/go/src/syscall/js/js_test.go:107:23: -1 << 40 (untyped int constant -1099511627776) overflows int
../../../../../../../usr/local/go/src/syscall/js/js_test.go:108:23: 1 << 60 (untyped int constant 1152921504606846976) overflows int
../../../../../../../usr/local/go/src/syscall/js/js_test.go:109:23: -1 << 60 (untyped int constant -1152921504606846976) overflows int
Exited with code 1

neelance · 2019-04-24T17:00:40Z

This is unfortunate. Okay, maybe coping js_test.go is the simplest solution.

hajimehoshi · 2019-04-24T17:41:06Z

OK, I copied the test, but now I'm facing a new error on my machine:

$ go generate ./... && go run . test syscall/js
writing fs_vfsdata.go
writing fs_vfsdata.go
gopherjs: Source maps disabled. Install source-map-support module for nice stack traces. See https://github.com/gopherjs/gopherjs#gopherjs-run-gopherjs-test.
testing: warning: no tests to run
PASS
[...]

Looks like the file is compiled Now it's midnight, I'll investigate this further later...

hajimehoshi · 2019-04-25T15:04:14Z

@neelance In the current implementation, is it possible to 'add' new tests in compiler/native?

hajimehoshi · 2019-04-25T15:17:26Z

is it possible to 'add' new tests in compiler/native?

I thought it impossible, so I moved js_test.go in compiler/native to tests/syscalljs/js_test.go. Also I found some tests that cannot pass, so I added t.Skip.

TestNaN: comparing NaN doesn't work with *js.Object
TestZeroValue: the initial value of *js.Object is nil, which is not undefined.

Please take a look

neelance · 2019-04-25T15:28:12Z

The zero value is quite important. I think it should be possible to handle this correctly, shouldn't it?

hajimehoshi · 2019-04-25T16:46:51Z

I think it should be possible to handle this correctly, shouldn't it?

Yes. I fixed that so that the zero value is js.Undefined.

I appreciate your patience and your support. Please take another look.

hajimehoshi · 2019-04-30T13:29:31Z

ping

johanbrandhorst · 2019-04-30T15:57:35Z

This is fantastic 🎉. Well done @hajimehoshi

hajimehoshi requested review from neelance, dmitshur and myitcv April 6, 2019 10:47

dmitshur approved these changes Apr 10, 2019

View reviewed changes

tests/syscalljs_test.go Outdated Show resolved Hide resolved

compiler/natives/fs_vfsdata.go Outdated Show resolved Hide resolved

hajimehoshi force-pushed the syscalljs branch from 94047f7 to a96317e Compare April 10, 2019 21:26

Add syscall/js

dfa4ee6

hajimehoshi force-pushed the syscalljs branch from a96317e to dfa4ee6 Compare April 11, 2019 00:33

Use the original tests

52d06cf

Add syscall/js test on CircleCI

fecd56c

Copy the original js_test.go

859138e

Move js_test.go to tests/syscalljs/js_test.go

3f13a79

hajimehoshi added 2 commits April 26, 2019 00:18

Remove unused code

1ff873a

gofmt

60dbf5a

hajimehoshi added 2 commits April 26, 2019 01:23

Make the zero value js.Undefined

aad5bc4

Add TestMain to suppress deadlock error

3b76189

neelance approved these changes Apr 30, 2019

View reviewed changes

hajimehoshi merged commit de1776f into master Apr 30, 2019

hajimehoshi deleted the syscalljs branch April 30, 2019 15:56

This was referenced May 18, 2019

v2: Create with support for both GopherJS and GOOS=js GOARCH=wasm. dominikh/go-js-dom#57

Closed

js: Consider adding Wasm support. #833

Open

flimzy mentioned this pull request May 20, 2019

Build tag to indicate syscall/js support? #920

Closed

emidoots mentioned this pull request Aug 15, 2020

Deprecate and remove GopherJS support from Vecty hexops/vecty#264

Closed

Add syscall/js #908

Add syscall/js #908

Uh oh!

Conversation

hajimehoshi commented Apr 6, 2019

Uh oh!

hajimehoshi commented Apr 6, 2019

Uh oh!

neelance commented Apr 6, 2019

Uh oh!

hajimehoshi commented Apr 6, 2019

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hajimehoshi commented Apr 11, 2019

Uh oh!

hajimehoshi commented Apr 17, 2019

Uh oh!

neelance commented Apr 17, 2019

Uh oh!

hajimehoshi commented Apr 18, 2019

Uh oh!

hajimehoshi commented Apr 18, 2019

Uh oh!

neelance commented Apr 23, 2019

Uh oh!

hajimehoshi commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neelance commented Apr 24, 2019

Uh oh!

hajimehoshi commented Apr 24, 2019

Uh oh!

hajimehoshi commented Apr 25, 2019

Uh oh!

hajimehoshi commented Apr 25, 2019

Uh oh!

neelance commented Apr 25, 2019

Uh oh!

hajimehoshi commented Apr 25, 2019

Uh oh!

hajimehoshi commented Apr 30, 2019

Uh oh!

johanbrandhorst commented Apr 30, 2019

Uh oh!

Uh oh!

hajimehoshi commented Apr 24, 2019 •

edited

Loading