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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

SamWhited
Copy link

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

Copy link
Contributor

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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
Copy link
Contributor

@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
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 mentioned this pull request Jan 8, 2019
5 tasks
@SamWhited SamWhited closed this Jan 8, 2019
@SamWhited SamWhited deleted the gomod_v3 branch January 8, 2019 06:15
@SamWhited
Copy link
Author

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
Copy link
Member

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants