Add support for source maps that use the local disk path of go source #521

Merged
merged 4 commits into from Sep 30, 2016

Projects

None yet

3 participants

@nullstyle
Contributor

This PR adds a simple command line flag to the gopherjs tools that configures how the sourcemaps are generated during a build. When using the --localmap flag, the sourcemaps will use the original local disk location instead of the default behavior.

This improves the development experience when serving gopherjs compiled code off the local disk, such as, in my case running an electron app powered by gopherjs.

@shurcooL

Hmm.

I suppose I see the use case for this. But I wonder if there's some way of achieving this without adding a new flag and the plumbing.

I wonder what @neelance thinks of this.

If there's really no better way to achieve this goal, then I'm okay with adding this.

It's also good to think about how to achieve this in a post-#388 world. If we were calling GOARCH=js go build -compiler=js, how would you specify this option to the compiler? Any thoughts @nullstyle.

Thanks for contributing!

build/build.go
@@ -672,21 +673,26 @@ func (s *Session) WriteCommandPackage(archive *compiler.Archive, pkgObj string)
return compiler.WriteProgramCode(deps, sourceMapFilter)
}
-func NewMappingCallback(m *sourcemap.Map, goroot, gopath string) func(generatedLine, generatedColumn int, originalPos token.Position) {
+func NewMappingCallback(m *sourcemap.Map, goroot, gopath string, localmap bool) func(generatedLine, generatedColumn int, originalPos token.Position) {
@nullstyle
nullstyle Sep 20, 2016 Contributor

Duh, totally. Thanks for the catch. I'll update it momentarily.

@nullstyle
Contributor

@shurcooL regarding GOARCH=js go build -compiler=js, I'm not entirely sure. I wasn't aware that was a possibility for invoking gopherjs (brand new gopherjs noob here), but I'll look into it.

I'm totally cool with trying to find a cleaner way to pull this off. I mostly just hacked this PR together to get some feedback and guidance since I'm completely new to this code base.

As one possible alternative... we could provide a different subcommand that (or a separate tool) that rewrites a source map to use the local disk files.

@nullstyle
Contributor

@shurcooL On 1.7, I'm getting the error below when trying to use to go tool to build against gopherjs. Is there something I'm missing?

$ GOARCH=js go build -compiler=js .
invalid value "js" for flag -compiler: unknown compiler "js"
@shurcooL
Member
shurcooL commented Sep 20, 2016 edited

Sorry if I mislead you, but we're not in a post-#388 world yet. That issue is still an open proposal. It's not complete. See #485 for a WIP PR for it.

@nullstyle
Contributor

Ah, I see, thanks for the heads up.

IMO, in a post-#388 world the most appropriate place to put the "local source maps" option would be as a -gcflags option. I don't have any experience using gcflags myself, but have found evidence that it's the appropriate place to modify compiler behavior: http://dave.cheney.net/2012/10/07/notes-on-exploring-the-compiler-flags-in-the-go-compiler-suite

@shurcooL
Member

Yep, that's what I was thinking, -gcflags or something along those lines.

@neelance, what do you think about this PR?

@neelance

The approach here looks fine to me. The requirement is valid and the implementation seems simple to me.

Please see comment for a change that I'd like to see before merging.

tool.go
@@ -588,7 +592,7 @@ func (fs serveCommandFileSystem) Open(requestName string) (http.File, error) {
sourceMapFilter := &compiler.SourceMapFilter{Writer: buf}
m := &sourcemap.Map{File: base + ".js"}
- sourceMapFilter.MappingCallback = gbuild.NewMappingCallback(m, fs.options.GOROOT, fs.options.GOPATH)
+ sourceMapFilter.MappingCallback = gbuild.NewMappingCallback(m, fs.options.GOROOT, fs.options.GOPATH, false)
@neelance
neelance Sep 24, 2016 Member

For consistency, please also add the option to the serve command and the others where it applies.

@nullstyle
nullstyle Sep 24, 2016 Contributor

Thanks for the heads up. Fixed.

@neelance
neelance Sep 25, 2016 Member

I think AddFlag is still missing for serve and the other commands.

@nullstyle
nullstyle Sep 29, 2016 Contributor

doy. sorry, I didn't read very well last time.

I've pushed a change that adds --localmap support to other commands: specifically install, test, serve, and get. Being a gopherjs noob, I wasn't quite sure how this flag would apply to gopherjs get, but it mentioned removing a .map file when browsing the source so I included it in this change. Please advise whether I should remove it.

@shurcooL
shurcooL Sep 29, 2016 Member

gopherjs get is equivalent to go get -d -tags=js followed by gopherjs install. If gopherjs install needs it, then so does gopherjs get.

@neelance neelance merged commit 5987515 into gopherjs:master Sep 30, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@neelance
Member

LGTM. Thanks, @nullstyle!

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