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

Support Go module #42

Closed
aofei opened this issue Nov 2, 2018 · 5 comments
Closed

Support Go module #42

aofei opened this issue Nov 2, 2018 · 5 comments

Comments

@aofei
Copy link
Contributor

aofei commented Nov 2, 2018

Hi @tdewolff,

I think you misunderstood the solution I mentioned in #41. So I opened this issue.

In fact, we don't need to release v3 to allow both tdewolff/parse and tdewolff/minify to support the Go module. And the /v2 suffix is mandatory, not optional.

Please take a look at the changes I made in my fork (aofei/parse). I tested it and it works fine (for both v1 and v2).

The changes:

  1. Create branch v1
  2. Commit 4049502
  3. Release v1.1.1 for the 4049502
  4. Create branch v2
  5. Commit 0975233
  6. Release v2.3.5 for the 0975233

I learned from russross/blackfriday.

@tdewolff
Copy link
Owner

tdewolff commented Nov 5, 2018

Thanks for your time @aofei, do you think we can make this work without updating the v1 branch? Version 1 is out-dated an unsupported and should not be receiving updates. I'm afraid that users with an older Go version might get reverted to version 1 if they don't update their import paths.

Additionally, while a new v3 might not be necessary, as import paths need to be updated this is a major breaking change which should bring out a new major version. But I guess we can just ignore that though and keep it on v2...that could work for me.

I'm also quite inclined to ignore Go modules as it seems to handle libraries versioned like mine poorly, does it currently not just grab the latest version since there is no go.mod anymore?

EDIT: Let's see what they say: https://www.reddit.com/r/golang/comments/9u9pt6/go_modules_start_at_v2/

@aofei
Copy link
Contributor Author

aofei commented Nov 5, 2018

It seems that the Go module tool has checked out the latest tag (v2.3.4). Since it has a go.mod file inside, and its module path is invalid (missing /v2 suffix), so... crack...

Releasing a new version (maybe v2.3.5) without a go.mod file seems to temporarily solve the check out problem. But that is definitely not a good solution, isn't it? I mean, after all, the Go module will become more and more supported anyway. Why not start supporting now?

In fact, changes to v1 can be ignored. Just need to consider v2.

...
I'm afraid that users with an older Go version might get reverted to version 1 if they don't update their import paths.
...

Emmm, yeah, this will be a very serious consequence...

...
This section so far has been focused on code that opts in to modules. However, putting major versions in import paths for v2+ modules could create incompatibilities with older versions of Go, or with code that has not yet opted in to modules. To help with this, Go versions 1.9.7+, 1.10.3+ and 1.11 have been updated so that code built with those releases can properly consume v2+ modules without requiring modification of pre-existing code. (When relying on this updated mechanism, a package that has not opted in to modules would not include the major version in the import path for any imported v2+ modules. In contrast, a package that has opted in to modules must include the major version in the import path for any imported v2+ modules).
...

According to the Modules, section Semantic Import Versioning, it's not a big problem for the Go >= v1.9. But for older versions, seems that they will all fail to run go get.

I personally think that changing the import path is acceptable. After all, it's now a problem, and it will still be a problem in the future (even if v3 is released). Or, freeze the master branch (for older Go versions, just like the russross/blackfriday), and only commit new changes to the v2 branch in the future. In this way, nothing will break. But that would be weird.

@tdewolff
Copy link
Owner

tdewolff commented Nov 6, 2018

This should now be fixed by using the github.com/tdewolff/parse/v2 import path!

@tdewolff tdewolff closed this as completed Nov 6, 2018
@aofei
Copy link
Contributor Author

aofei commented Nov 6, 2018

Awesome! I believe that you have made a very good choice.

Now, I think it's time to fix tdewolff/minify.

@tdewolff
Copy link
Owner

tdewolff commented Nov 6, 2018

Should be fixed, see https://github.com/tdewolff/minify/releases/tag/v2.3.7!!

aofei added a commit to aofei/air that referenced this issue Nov 6, 2018
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

No branches or pull requests

2 participants