Skip to content

Conversation

@nitram509
Copy link
Contributor

@nitram509 nitram509 commented Nov 30, 2022

This PR replaces one aspect of the #17 PR.

with Go 1.18, the js.Wrapper is no more available, so I removed it. See https://tip.golang.org/doc/go1.18

This will fix #16

@ptxmac
Copy link

ptxmac commented Nov 30, 2022

This will fix go1.18+, but what about users still on older versions of go, will they suddenly get different behavior?

Perhaps it's better to split value into two files and use build-tags so js.Wrapper can still be supported on older go versions

@nitram509
Copy link
Contributor Author

@ptxmac I'm sorry, I lack details of the former & discontinued Go version.
In theory, I guess, there's a little chance of breaking behaviour.
I hesitate to adopt this PR with compiler flags and would like to ask for guidance & clarity first.

Where I have no clear understanding (and just a personal opinion) is, if support for the older Go version is a strong requirement. I have two things in mind.
a) does the vert library state a goal, to support the former Go version with every release?
b) how many users are still on the former Go version?

Regarding a) I can't speak for this project (being just a contributor) but hoping @norunners or @jasikpark or other maintainers could give guidance here. E.g. is there a view on how many releases in the past of Go are supported by a vert release? There could be an answer, that vert would release a "v2" which would be Go1.18+, and for clients using older Go versions, they would not be able to use new releases. Being curious to hear their feedback.

Regarding b) I see 15 projects listed by Github using this lib. One could simply check how up-to-date they are. Maybe there are no such users sticking on 1.17 or older ;)

@ptxmac
Copy link

ptxmac commented Nov 30, 2022

I also haven't looked into what js.Wrapped did in pre 1.17

I'm all for removing legacy support, but then maybe it makes sense to upgrade the target in go.mod so anyone still using older versions will be unable to use this version?

@jasikpark
Copy link

might be worth adding tests that show the difference in functionality ig? not a maintainer of vert, just interested in seeing go 1.18+ support get off the ground 😃

@norunners
Copy link
Owner

Thank you again for the contributions.

I'm wondering if this usage of js.Wrapper needs to be revisited. I'll take a closer look as needed.

  • vert/value.go

    Lines 22 to 25 in 3a6ecdd

    // JSValue returns the JS value.
    func (v Value) JSValue() js.Value {
    return v.Value
    }

@ptxmac
Copy link

ptxmac commented Dec 2, 2022

I had a look at the original proposal as to why it was removed golang/go#44006 and a proposal to reintroduce parts of it 😄 golang/go#50310

Since there's no longer anything in the syscall/js package that uses JSValue() the method does seem redundant. That said, other packages might declare their own interface for "js wrappable objects" and it would make sense that many packages agree on reusing the js.Wrapper interface for compatibility.

@jasikpark
Copy link

gowebapi/webapi#15 is another project that has transitioned away from it

@norunners
Copy link
Owner

norunners commented Dec 3, 2022

Alright, I thought about this a bit. The way that this can break consumers is a bit of an edge case, the call would look likejs.ValueOf(vert.Value), where the js.Wrapper interface used to be used under the hood in js.ValueOf. After this change, the result of such a call will be a panic since the struct kind is unsupported by js.ValueOf. The fix of such a call should be js.ValueOf(vert.Value.JSValue()). Due to this situation, it probably makes sense to keep the vert.Value.JSValue method around. At this point, I'm okay to merge the change.

@norunners norunners merged commit 106a353 into norunners:main Dec 3, 2022
@jasikpark
Copy link

thx for merging this quickly! i'll be excited now to be able to help update withastro/compiler to go 1.19 now that this supports it

fuzzing a compiler time :)

@nitram509 nitram509 deleted the fix/go-1.19-jsWrapper-issue branch December 4, 2022 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build failures under Go 1.18 due to js.Wrapper removal

4 participants