hexops / vecty

Vecty lets you build responsive and dynamic web frontends in Go using WebAssembly, competing with modern web frameworks like React & VueJS.
BSD 3-Clause "New" or "Revised" License
2.8k stars 143 forks source link

incorrect-looking // +build line #261

Closed rsc closed 4 years ago

rsc commented 4 years ago

I've been analyzing // +build usage in the Go ecosystem and turned up dom_wasmjs_gopherjs.go, which says:

// +build go1.12,wasm,js js

and dom_native.go which says:

// +build go1.12,!wasm,!js

These two conditions are:

dom_wasmjs_gopherjs.go: (go1.12 && wasm && js) || js which is a complex way of writing js. dom_native.go: go1.12 && !wasm && !js

I am unsure about what exactly is intended here. From the comments in dom_native.go, it looks like the intent was for dom_wasmjs_gopherjs.go to apply to js/wasm builds and dom_native.go to apply otherwise. That would be:

dom_wasmjs_gopherjs.go: go1.12 && js && wasm

// +build go1.12,js,wasm

dom_native.go: !(go1.12 && js && wasm) which is !go1.12 || !js || !wasm

// +build !go1.12 !js !wasm

Best, Russ

slimsag commented 4 years ago

Thanks Russ and apologies for the delayed response. The intention you described is correct:

it looks like the intent was for dom_wasmjs_gopherjs.go to apply to js/wasm builds and dom_native.go to apply otherwise.

However, it misses one point which was that we are also intending here to support the GopherJS compiler which does not set the wasm build tag. Thus your suggestion go1.12 && js && wasm:

// +build go1.12,js,wasm

Would work in the case of GOOS=wasm GOARCH=js go build but not in the case of gopherjs build which doesn't set the wasm build tag.

You are right that this is true:

dom_wasmjs_gopherjs.go: (go1.12 && wasm && js) || js which is a complex way of writing js.

I wrote it this way in hopes of being a bit more explicit:

But given this issue I presume that just js and a comment would've been better perhaps :)

(A bit unrelated, but this is another example of why https://github.com/gopherjs/vecty/issues/264 is probably a good call at this point.)

slimsag commented 4 years ago

Fixed by https://github.com/hexops/vecty/pull/267