Add support for source maps that use the local disk path of go source #521
Conversation
There was a problem hiding this comment.
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!
| @@ -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) { | |||
There was a problem hiding this comment.
Style-wise, this should be localMap. See https://github.com/golang/go/wiki/CodeReviewComments#mixed-caps.
There was a problem hiding this comment.
Duh, totally. Thanks for the catch. I'll update it momentarily.
|
@shurcooL regarding 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. |
|
@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" |
|
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 |
|
Yep, that's what I was thinking, @neelance, what do you think about this PR? |
| @@ -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) | |||
There was a problem hiding this comment.
For consistency, please also add the option to the serve command and the others where it applies.
There was a problem hiding this comment.
Thanks for the heads up. Fixed.
There was a problem hiding this comment.
I think AddFlag is still missing for serve and the other commands.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
gopherjs get is equivalent to go get -d -tags=js followed by gopherjs install. If gopherjs install needs it, then so does gopherjs get.
|
LGTM. Thanks, @nullstyle! |
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
--localmapflag, 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.