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

Bump module version to v3 (again) now that support is backported #364

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@SamWhited
Copy link

SamWhited commented Nov 26, 2018

Hello,

Discussion in #302 concluded by removing support for Go Modules (without the "incompatible" workaround) because the /v3 suffix was only supported by users of vgo. However, since then limited support for importing using the version suffix has been backported to older versions of Go (including 1.9.7+ and 1.10.3+). For more information see here: https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher

I would like to request that the /v3 suffix be added back to the module and the import path.
This does require that support for Go 1.7 and 1.8 be dropped, but they haven't been supported for several versions anyways.

This will also (EDIT: this should have read NOT break) break existing import paths, which was another reason it was reverted previously, however, since it's an easy update I am hoping you will reconsider now that it is supported,

Thank you for your consideration.

@VojtechVitek
Copy link
Contributor

VojtechVitek left a comment

Hi @SamWhited,
thanks for the contribution. However, we've been through this already, see #302 (comment) and its related PRs.

We don't want to ever break import paths of chi v3.x (by adding /v3 suffix) and break our users.
Thus, we'll stay on +incompatible flag, until v4.x is released (not planned at this point).

Pls, let me know if I'm missing something.

@SamWhited

This comment has been minimized.

Copy link
Author

SamWhited commented Nov 26, 2018

I'm sorry, my previous message was unclear and had a statement that was incorrect: Go 1.9.7+ and Go 1.10.3+ (eg. one version before the last supported version, which is 1.10) both support module import paths now. Even if you leave the path off, it will not break things. For example, your example from the documentation builds fine on Go 1.9.7, even with this PR merged.

This will NOT break compatibility with existing code.

eg. the following session can be run with this branch:

$ pwd
/home/example/go/src/chiexample
$ cat main.go 
package main

import (
        "net/http"

        "github.com/go-chi/chi"
        "github.com/go-chi/chi/middleware"
)

func main() {
        r := chi.NewRouter()
        r.Use(middleware.Logger)
        r.Use(middleware.Recoverer)

        r.Get("/", func(w http.ResponseWriter, r *http.Request) {
                w.Write([]byte("root."))
        })

        http.ListenAndServe(":3333", r)
}
$ GO111MODULE=off go get -u golang.org/dl/go1.9.7
$ go1.9.7 download
<snip>
$ go1.9.7 build
$ ls -la chiexample 
-rwxr-xr-x 1 example example 8944294 Nov 26 17:34 chiexample
@VojtechVitek

This comment has been minimized.

Copy link
Contributor

VojtechVitek commented Nov 27, 2018

@SamWhited You are assuming that chi will support Go 1.9.7+ and Go 1.10.3+ only from now on .. and that we'll silently drop support for 1.7, 1.8, 1.9.0, 1.10.0. We need community consensus on this -- please create an issue for this. This PR is indeed breaking backward compatibility.

RE: Import paths .. thanks for the explanation. So even if we don't add the /v3 suffix, the import paths will work just fine? So why add the suffix in the first place, should this PR avoid adding it then?

@SamWhited

This comment has been minimized.

Copy link
Author

SamWhited commented Nov 27, 2018

You are assuming that chi will support Go 1.9.7+ and Go 1.10.3+ only from now on .. and that we'll silently drop support for 1.7, 1.8, 1.9.0, 1.10.0. We need community consensus on this -- please create an issue for this. This PR is indeed breaking backward compatibility.

I had assumed those had just been left in the test matrix but weren't actually supported given that the Go team only supports the last two versions of Go. I'll open an issue.

RE: Import paths .. thanks for the explanation. So even if we don't add the /v3 suffix, the import paths will work just fine? So why add the suffix in the first place, should this PR avoid adding it then?

This is for backwards compatibility, it works this way when using versions that don't have full module support. In go 1.9.7 we know where the library is (because we're looking it up in the GOPATH), and we can't import multiple versions of it without renaming the imports anyways, so it effectively just ignores the version suffix (if it's not an actual package that was named v3 or whatever).

With Go 1.11+ however, it doesn't use GOPATH so it uses the version suffix to decide which package to download.

@pkieltyka pkieltyka referenced this pull request Jan 8, 2019

Merged

chi v4 #378

4 of 5 tasks complete

@SamWhited SamWhited closed this Jan 8, 2019

@SamWhited SamWhited deleted the SamWhited:gomod_v3 branch Jan 8, 2019

@SamWhited

This comment has been minimized.

Copy link
Author

SamWhited commented Jan 10, 2019

We don't want to ever break import paths of chi v3.x (by adding /v3 suffix) and break our users.
Thus, we'll stay on +incompatible flag, until v4.x is released (not planned at this point).

Hello,

Chi v4 was released, but it still doesn't have module support. Does this mean modules and the proper import path won't be supported until v5 for compatibility?

(also asked on #378; sorry, I wasn't sure where the appropriate place to ask was)

@pkieltyka

This comment has been minimized.

Copy link
Member

pkieltyka commented Jan 10, 2019

@SamWhited yea, it would be chi v5 when we support go modules. I don't see this happening for another 6+ months until go 1.13 is available, in hope the go modules is standard and robust for packages. We had hopes to use go modules now, but ran into a bunch of complexity that didn't make sense to put onto chi which literally has zero dependencies to run, lol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment