Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Support Go 1.13 and Go 1.14 both #964
Conversation
…t: update for Go1.13
…nge go version request rebuild packages
It's failing because the |
Any updates on this PR? Thanks! |
Hi @dmitshur, @visualfc and everyone else! I was suddenly made aware that Go 1.15 is practically around the corner, so soon enough we'll have to begin worrying about supporting that :) Addressing Dmitri's question in #964 (comment) on how to proceed with version branches, I'd like to suggest skipping 1.13 entirely and merging this PR as Go 1.14 support. This PR is pretty large, though, so I would like to offer my help reviewing it, if would help moving along and everyone is ok with that. |
I'm finished the first pass over the code and decided to publish the comments I have so far :) As a general comment, this PR attempts to support 3 Go versions simultaneously, but this seems to add a noticeable amount of complexity. Given that the amount of time GopherJS maintainers' seem to have available these days, it seems to set the project up for increased burden in future. I'd suggest dropping support for Go 1.12 and 1.13, and going with just Go 1.14. Given that 1.15 is coming soon, the value of keeping support for the older versions is quickly diminishing. |
@@ -17,7 +17,7 @@ import ( | |||
|
|||
var sizes32 = &types.StdSizes{WordSize: 4, MaxAlign: 8} | |||
var reservedKeywords = make(map[string]bool) | |||
var _ = ___GOPHERJS_REQUIRES_GO_VERSION_1_13___ // Compile error on other Go versions, because they're not supported. | |||
var _ = ___GOPHERJS_REQUIRES_GO_VERSION_1_12___ // Compile error on other Go versions, because they're not supported. |
// // Zero returns a Value representing the zero value for the specified type. | ||
// // The result is different from the zero value of the Value struct, | ||
// // which represents no value at all. | ||
// // For example, Zero(TypeOf(42)) returns a Value with Kind Int and value 0. | ||
// // The returned value is neither addressable nor settable. | ||
// func Zero(typ Type) Value { | ||
// if typ == nil { | ||
// panic("reflect: Zero(nil)") | ||
// } | ||
// t := typ.(*rtype) | ||
// fl := flag(t.Kind()) | ||
// if ifaceIndir(t) { | ||
// return Value{t, unsafe_New(t), fl | flagIndir} | ||
// } | ||
// return Value{t, nil, fl} | ||
// } | ||
|
||
// // ToInterface returns v's current value as an interface{}. | ||
// // It is equivalent to: | ||
// // var i interface{} = (v's underlying value) | ||
// // It panics if the Value was obtained by accessing | ||
// // unexported struct fields. | ||
// func ToInterface(v Value) (i interface{}) { | ||
// return valueInterface(v) | ||
// } | ||
|
||
// type EmbedWithUnexpMeth struct{} | ||
|
||
// func (EmbedWithUnexpMeth) f() {} | ||
|
||
// type pinUnexpMeth interface { | ||
// f() | ||
// } | ||
|
||
// var pinUnexpMethI = pinUnexpMeth(EmbedWithUnexpMeth{}) | ||
|
||
// func FirstMethodNameBytes(t Type) *byte { | ||
// _ = pinUnexpMethI | ||
|
||
// ut := t.uncommon() | ||
// if ut == nil { | ||
// panic("type has no methods") | ||
// } | ||
// m := ut.methods()[0] | ||
// mname := t.(*rtype).nameOff(m.name) | ||
// if *mname.data(0, "name flag field")&(1<<2) == 0 { | ||
// panic("method name does not have pkgPath *string") | ||
// } | ||
// return mname.bytes | ||
// } | ||
|
||
// type Buffer struct { | ||
// buf []byte | ||
// } |
// func StructOf(fields []StructField) Type { | ||
// jsFields := make([]*js.Object, len(fields)) | ||
// fset := map[string]struct{}{} | ||
// for i, f := range fields { | ||
// if f.Type == nil { | ||
// panic("reflect.StructOf: field " + strconv.Itoa(i) + " has no type") | ||
// } | ||
|
||
// name := f.Name | ||
// if name == "" { | ||
// // Embedded field | ||
// if f.Type.Kind() == Ptr { | ||
// // Embedded ** and *interface{} are illegal | ||
// elem := f.Type.Elem() | ||
// if k := elem.Kind(); k == Ptr || k == Interface { | ||
// panic("reflect.StructOf: illegal anonymous field type " + f.Type.String()) | ||
// } | ||
// name = elem.String() | ||
// } else { | ||
// name = f.Type.String() | ||
// } | ||
// } | ||
|
||
// if _, dup := fset[name]; dup { | ||
// panic("reflect.StructOf: duplicate field " + name) | ||
// } | ||
// fset[name] = struct{}{} | ||
|
||
// jsf := js.Global.Get("Object").New() | ||
// jsf.Set("prop", name) | ||
// jsf.Set("name", name) | ||
// jsf.Set("exported", true) | ||
// jsf.Set("typ", jsType(f.Type)) | ||
// jsf.Set("tag", f.Tag) | ||
// jsFields[i] = jsf | ||
// } | ||
// return reflectType(js.Global.Call("$structType", "", jsFields)) | ||
// } |
vendor/golang.org/x/crypto/chacha20poly1305 | ||
vendor/golang.org/x/crypto/cryptobyte | ||
vendor/golang.org/x/crypto/cryptobyte/asn1 | ||
vendor/golang.org/x/crypto/curve25519 | ||
vendor/golang.org/x/crypto/hkdf | ||
vendor/golang.org/x/crypto/internal/chacha20 | ||
vendor/golang.org/x/crypto/internal/subtle | ||
vendor/golang.org/x/crypto/poly1305 | ||
vendor/golang.org/x/net/dns/dnsmessage | ||
vendor/golang.org/x/net/http/httpguts | ||
vendor/golang.org/x/net/http/httpproxy | ||
vendor/golang.org/x/net/http2/hpack | ||
vendor/golang.org/x/net/idna | ||
vendor/golang.org/x/net/nettest | ||
vendor/golang.org/x/net/route | ||
vendor/golang.org/x/sys/cpu | ||
vendor/golang.org/x/text/secure/bidirule | ||
vendor/golang.org/x/text/transform | ||
vendor/golang.org/x/text/unicode/bidi | ||
vendor/golang.org/x/text/unicode/norm |
nevkontakte
Aug 2, 2020
Contributor
This is a bit strange. To my understanding, this list should only cover standard library, so seeing vendored packages is a bit unexpected.
"github.com/fsnotify/fsnotify" | ||
"github.com/gopherjs/gopherjs/compiler" | ||
"github.com/gopherjs/gopherjs/compiler/gopherjspkg" | ||
"github.com/gopherjs/gopherjs/compiler/natives" | ||
"github.com/neelance/sourcemap" | ||
"github.com/shurcooL/httpfs/vfsutil" | ||
"golang.org/x/tools/go/buildutil" | ||
|
||
"github.com/visualfc/fastmod" |
nevkontakte
Aug 2, 2020
Contributor
As a general observation, lack of comments makes understanding this PR considerably harder, in particular in the part that's related to modules support, the fastmod
package godocs are empty.
This, unfortunately, can be said about GopherJS codebase in general too :-(
Archives map[string]*compiler.Archive | ||
Types map[string]*types.Package | ||
Watcher *fsnotify.Watcher | ||
} | ||
|
||
func (s *Session) checkMod(pkg *PackageData) (err error) { |
nevkontakte
Aug 2, 2020
Contributor
I think I commented on this on the other PR… Unless I'm misunderstanding the purpose of this function it facilitates passing some module-related information through a side channel, which is a generally problematic pattern (for all the same reasons why global variables are to be avoided).
Aside from that, it creates a risk of hard to debug errors if there's a codepath in which this function is not called.
for _, src := range build.Default.SrcDirs() { | ||
dir := filepath.Join(src, importPath) | ||
if _, err := os.Stat(dir); err == nil { | ||
return dir | ||
} | ||
} |
nevkontakte
Aug 2, 2020
Contributor
Why is this needed? I'd expect build.Import() to find packages in standard locations. Same below.
@@ -0,0 +1,1048 @@ | |||
// +build js |
nevkontakte
Aug 2, 2020
Contributor
So far I tool only cursory look at reflectlite
code. I assume it is mostly derived from GopherJS version of reflect
?
Is it possible to somehow reduce the amount of code duplication between the two?
I'm getting a feeling like I'm talking to the void here :-/ @hajimehoshi @dmitshur @visualfc Is there any interest in merging this PR at all? Any way I can help advancing this? |
You are correct in your deduction. The project is dead and the maintainers don't care (they have other priorities). I have an ignored PR too but I've known it's been dead for 1+ years. It's really time for a hard fork for people such as you and I that actually use it in prod. They'll never transfer ownership of repo. I am the owner of https://github.com/rocketlaunchr/react repo which I use in prod and which relies on gopherjs. If I understood the internals, I would hard-fork it in a second. What I consider to be disrespectful is that the maintainers don't declare that project is dead in the README, leading people like you to spend many many days/weeks issuing PR's, when they know they will be ignored. Finally it's hard/unwise to call them out because some of them are integral members of the Go team. |
Seems pretty much so. |
To be clear, I recognize that GopherJS maintainers may be busy with other things (work, life, etc) and have little or no time to dedicate to the project at this time, but I'd like to hear at least some statement of intentions related to this PR. A hard fork would be very unfortunate and I'd very much prefer to avoid it. Cheers! |
Sorry but currently I don't have bandwidth to care this project. Moreover, I'm not familiar with the internal details of GopherJS compared to @dmitshur and the other maintainers.
Actually there are forks like https://github.com/myitcv/gopherjs, which has already merged some PRs in this project. |
this pr work for go1.14, but not support go1.15. seem support go1.15 is hard. |
@hajimehoshi would it be possible to nominate a successor fork in the readme? |
I also use this in production. @dmitshur can you please bring some clarity in here? |
I think it would be better to see if @neelance and @dmitshur would be willing to promote one or two new owners for this project. WDYT? |
I fully agree with this. The project is already quite complex in what it does, it is better to avoid any additional complexity. @nevkontakte I haven't talked to @dmitshur yet, but would you be open to being a maintainer of this project? You seem to be invested in GopherJS and I appreciate your code review. |
@neelance thanks for the offer. The amount of spare time I have is also limited, but I do indeed have a few projects in production that rely on GopherJS. If there's no better volunteer available, I'm happy to join :) |
Any news? I have multiple reasons to prefer GopherJS over Go/WASM, but at some point being able to use an up to date version will outweigh the advantages of GopherJS and the headache of migration :-( |
If someone hard forks, I'm happy to migrate over |
@nevkontakte this is from Slack: |
:( Sounds like a fork is in order, then. |
Yes, feel free to create a fork. If it is well maintained for a while, then we can endorse it or maybe even consider a merge at some point. But first Dmitri and I would like to see that the maintenance situation turns out better than what we are currently able to provide. :) |
what should it be called? |
I suggest we either open a new issue, and/or open a slack conversation about this, and other housekeeping details (which organization, who has commit access, etc, etc) so we don't clutter this PR, and so we have a canonical place to point people for posterity. WDYT? |
support work on Go1.15 https://github.com/visualfc/gopherjs/tree/go1.15-dev |
@visualfc Can you merge it to master in your fork so I can use it? I think it's time to adopt yours as the "official" gopherjs. Thanks for the amazing work. |
https://github.com/goplusjs/gopherjs for supported Go 1.12~1.15
|
Does it pass all the go unit tests? |
https://github.com/goplusjs/gopherjs |
Build on Go 1.13 / Go 1.14 and one build support working on Go 1.12 Go 1.13 Go 1.14 three version.
gopherjs build -a -v
orgopherjs test -a -v