b is not a descriptive variable name. That's often okay in short functions, but it doesn't work well in this context. It's also typically associated with []byte type, not io.Reader. I see r is already taken and I don't think it's a good idea to reuse it.

I think it's better to either use a longer descriptive name here:

exportData := bytes.NewReader(a.ExportData)
packages[path], err = gcexportdata.Read(exportData, token.NewFileSet(), packages, path)

Which would mirror the gcexportdata.Write code below.

But I think this line is not that long, that it's even better to just inline it:

packages[path], err = gcexportdata.Read(bytes.NewReader(a.ExportData), token.NewFileSet(), packages, path)

Either of those is okay.

Done.

This line uses a very outdated style of initializing a new bytes.Buffer, no one writes it like that in anymore. bytes.NewBuffer documentation hints at this:

In most cases, new(Buffer) (or just declaring a Buffer variable) is sufficient to initialize a Buffer.

It's not surprising because this line was written 4 years ago.

I know it's not your code, but it's the line directly following your code, and I think it's not nice how just 4 lines above it is a different style is used to do the same thing:

exportData := new(bytes.Buffer)
// ... 3 lines
encodedFileSet := bytes.NewBuffer(nil)

Let's make this better by updating it so they both use the same style.

I would slightly prefer the var buf bytes.Buffer (and using & operator) style over buf := new(bytes.Buffer):

var exportData bytes.Buffer
if err := gcexportdata.Write(&exportData, nil, typesPkg); err != nil {
	return nil, fmt.Errorf("failed to write export data: %v", err)
}
var encodedFileSet bytes.Buffer
if err := fileSet.Write(json.NewEncoder(&encodedFileSet).Encode); err != nil {

But if you feel strongly about using new(bytes.Buffer) for both variables (and not using & operator), I'm okay with that too.

Also, I know this file has yet another instance of bytes.NewBuffer(nil), but it's fine to leave that alone, it's too far from the affected code and hence out of scope of this PR.

Done.

Removing the now-unused old gcimporter15 package from vendor directory is the right thing to do. However, we shouldn't vendor the new gcexportdata package.

This project currently follows the pattern of vendoring as few packages as possible, only the ones that have to be vendored. The new gcexportdata package is not deprecated (nor removed from upstream) and doesn't need to be vendored at this time.

Not vendoring it means when newer version of gcexportdata is released, installing the latest version of GopherJS will automatically use the latest (otherwise, it would not be updated until we manually do so). This is intentional. At this time, we prefer the reduced maintenance burden by not vendoring when it's not required.

This PR should follow the current pattern and not deviate from it.

In the PR description, you said:

we instead vendor golang.org/x/tools/go/gcexportdata and
golang.org/x/tools/go/internal/gcimporter to avoid hassle for people
upgrading to this version of GopherJS.

I don't understand what hassle you're referring to. To update to a newer version of GopherJS, per the installation instructions, people do go get -u github.com/gopherjs/gopherjs. That automatically gets the latest versions of dependencies. Please let me know if I'm misunderstanding something, but I don't believe there is an increased hassle for users. (And if there is, we should deal with this outside of this PR if it involves other non-vendored packages too.)

So, please remove the newly vendored packages from this PR, just keep the deletion of gcimporter15.

I've removed the vendor. The situation I was referring to in the commit message is for anyone, like me, who might update the GopherJS repo in isolation. But it's a minor point and not necessarily an improvement in the whole so I've removed it.