playground: Add ability to fix imports via goimports.

[dmitshur]

Resolves #29.

This is a WIP, there are still a few things I want to clean up and do. Edit: All done.

• [x] Include gopherjs.com/gopherjs/gopherjs/js package in the pre-baked list.
  • Done in e6786f2e96137970f64bf94bd81fd64f47db2e79.
• [x] Revisit and ensure imports functionality is up to date and cannot be used from upstream.
  • When I originally wrote this code a while ago, I found that it was necessary to copy and modify imports package code to avoid reading from disk, because some private internal configuration had to be changed to make that work. Just want to confirm that's still the case.
  • Done in 0b05872fac99bb99220c1429ee89af04f74d12b5. I've confirmed it's necessary to make a copy of upstream package in order to disable/remove the code that uses disk IO to initialize pkgIndex and causes syscall errors in browser environment.
• [x] Move imports package under internal subfolder. We probably don't want others to be importing it.
  • Done in cb4ab587e0fd6e4e402690202f491b63acbabba8.
• [x] Build the playground JS code.
  • Done in 21947ff.

However, it's functional and ready for initial review.

[dmitshur]

Ok, everything is done from my side and this is ready to merge. It can be squashed into fewer commits after review.

PTAL.

[neelance]

What are the changes to the imports package? Is there a chance to get them upstream?

[dmitshur]

What are the changes to the imports package?

From the PR description (with emphasis added):

• [x] Revisit and ensure imports functionality is up to date and cannot be used from upstream.
  • When I originally wrote this code a while ago, I found that it was necessary to copy and modify imports package code to avoid reading from disk, because some private internal configuration had to be changed to make that work. Just want to confirm that's still the case.
  • Done in 0b05872fac99bb99220c1429ee89af04f74d12b5. I've confirmed it's necessary to make a copy of upstream package in order to disable/remove the code that uses disk IO to initialize pkgIndex and causes syscall errors in browser environment.

You can visualize the diff with any tool by comparing the folders of this imports package against golang.org/x/tools/imports. It looks something like this:

In summary, it removes some unused code (reducing the functionality to use in-memory pre-computed database of packages only, no falling back to reading from disk), and sets some internal configuration variables. Since those variables not exported, the upstream package cannot be directly used.

One alternative I see is not removing the unused code, but copying the upstream imports package as is, but adding a new config.go file that has:

func init() {
    // Override some internal funcs.
    importPathToName = importPathToNameBasic
    findImport = findImportStdlib
}

Which would make the forked package a little easier to maintain, but it will have a lot of extra unused code and potentially still cause syscall errors.

Is there a chance to get them upstream?

The problem is that the imports package does not use build tags to detect if it's running in an environment where doing direct disk IO is not a possibility.

One idea for how to resolve that upstream is to make a CL that makes the package friendly to such IO-less environments is to make use of the nacl build tag. I doubt the Go team will accept moving disk IO into a // +build !js file, but they may accept a // +build !nacl one.

However, even if that happens, I've built a custom index including Go 1.5 standard library and github.com/gopherjs/gopherjs/js package. This on its own means the upstream package cannot be used directly.

Concluding note

I realize this is a lot of code and it seems hard to maintain. However, I've considered that, and I think it will not cause a lot of problems for these reasons:

• Updating it is as simple as go generate github.com/gopherjs/gopherjs.github.io/playground/internal/imports.
• The imports package is mature and stable. It hasn't changed much in the last 2 years, it's unlikely to change much in the next 3.
• I will very likely maintain it for the next 3+ years at least.

[dmitshur]

I've squashed and rebased this PR into 4 logical commits, that should be easier to review, maintain and understand in the future.

This has LGTM and is ready to merge from my side.

[dmitshur]

@neelance, will you have a chance to review this soon? It's ready to merge from my side.

[neelance]

I think it is reasonable to ask for golang.org/x/tools/imports to support something like http://golang.org/pkg/go/build/#Context, which provides all functions for file system access. We should at least ask for it to show that there is a need. In the meantime vendoring should be okay.

[dmitshur]

I think it is reasonable to ask for golang.org/x/tools/imports to support something like http://golang.org/pkg/go/build/#Context, which provides all functions for file system access. We should at least ask for it to show that there is a need. In the meantime vendoring should be okay.

I will try, but a few things:

• build.Context isn't completely applicable here. It's a great abstraction for accessing known packages in a customizable way. But imports package works at a filesystem-access level before it can work at package level. It needs to walk the GOPATH and find all available packages, and build an index. It's not possible to walk the GOPATH and find a list of all existing Go packages via build.Context abstraction.
• Unless you meant something like build.Context but for imports package. Similar to Context of gotool package: https://godoc.org/github.com/kisielk/gotool#Context.

• So what's really needed is just a way to configure the internal options of imports package. Something like:

  // Or "Context" or "Configuration" or whatever name works best.
  type Config struct {
    ImportPathToName func(importPath string) (packageName string)
    LoadExports      func(dir string) map[string]bool
    FindImport       func(pkgName string, symbols map[string]bool) (string, bool, error)
  }
  
  func (c *Config) Process(filename string, src []byte, opt *Options) ([]byte, error) { ... }
  
  func Process(filename string, src []byte, opt *Options) ([]byte, error) {
    return defaultConfig.Process(filename, src, opt)
  }

• Ideally, things that rely on raw filesystem access and import packages like os can be hidden behind a build tag for systems that don't have filesystems (perhaps nacl build tag). Otherwise GopherJS will produce warnings for syscall package being imported in browser (unless it gets eliminated via DCE since it won't be used and that's enough to prevent the warning from happening, not sure if DCE would do that).

I'll file an issue with the feature request.

[dmitshur]

Filed issue https://github.com/golang/go/issues/12696.

With that, I've addressed all your outstanding comments. PTAL.

[neelance]

LGTM :)