Skip to content
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

Add syscall/js #908

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@hajimehoshi
Copy link
Member

commented Apr 6, 2019

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 hajimehoshi requested review from neelance, dmitshur and myitcv Apr 6, 2019

@hajimehoshi

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2019

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

Meh, this sounds like a flaky issue

@neelance

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2019

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

@dmitshur
Copy link
Member

left a comment

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.

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

@hajimehoshi hajimehoshi force-pushed the syscalljs branch from 94047f7 to a96317e Apr 10, 2019

@hajimehoshi hajimehoshi force-pushed the syscalljs branch from a96317e to dfa4ee6 Apr 11, 2019

@hajimehoshi

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

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

@hajimehoshi

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

ping @neelance

@neelance

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

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

@hajimehoshi

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

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

@hajimehoshi

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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

@hajimehoshi

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.