Add optional root argument to gopherjs serve. Fixes #384 #429

jargv · 2016-03-22T21:44:25Z

Fixes #384.

neelance · 2016-03-22T21:52:24Z

This looks really good, thanks for contributing! Two small wishes:

Make gopherjs help serve show the new optional argument.
Print the same help content if more than one argument is given.

dmitshur · 2016-03-23T04:57:08Z

Thanks for working on resolving that issue! I have some questions about the code changes.

dmitshur · 2016-03-23T04:57:27Z

tool.go

@@ -555,7 +571,7 @@ func (fs serveCommandFileSystem) Open(name string) (http.File, error) {
 	if isPkg || isMap || isIndex {
 		// If we're going to be serving our special files, make sure there's a Go command in this folder.
 		s := gbuild.NewSession(fs.options)
-		pkg, err := gbuild.Import(path.Dir(name[1:]), 0, s.InstallSuffix(), fs.options.BuildTags)
+		pkg, err := gbuild.Import(path.Dir(strings.Trim(name, "/")), 0, s.InstallSuffix(), fs.options.BuildTags)


What's the reason for this change?

When the first character was unconditionally dropped with name[1:] it was always a slash. With this change the first character is sometimes a slash, but not always. I was seeing it pass strings like ithub.com/jargv/... into path.Dir, which fails, of course. strings.Trim seemed like a good way to conditionally drop the first character if it's a '/'. I'm new(ish) to go so I'd love to learn if there's a more idiomatic approach. :)

With this change the first character is sometimes a slash, but not always.

I see. I asked I'd like to understand exactly in what situations it'a slash, and in what situations it's not.

strings.Trim seemed like a good way to conditionally drop the first character if it's a '/'. I'm new(ish) to go so I'd love to learn if there's a more idiomatic approach. :)

One immediate improvement suggestion would be to use strings.TrimLeft rather than strings.Trim, since you said you only want to drop the first character being /, not the last character. strings.Trim trims from both ends.

However, I understand what's happening better now, and I have a suggestion for how you can go back to just having an unconditional trim of first character (which should be simpler to reason about).

The signature of this method used to be Open(name string), and name[1:] used to be done to drop the first "/" that's a part of each *http.Request.URL.Path. Since you've re-arranged the code to be Open(requestName string), I think you can move the the [:1] operation to happen earlier:

func (fs serveCommandFileSystem) Open(requestName string) (http.File, error) { name := path.Join(fs.serveRoot, requestName[1:])

Then this line can become simply:

pkg, err := gbuild.Import(path.Dir(name), 0, s.InstallSuffix(), fs.options.BuildTags)

What do you think of that?

Yes, I agree TrimLeft would be better, and the move you suggest would be better still! I'll make that change shortly.

dmitshur · 2016-03-23T04:59:19Z

How does this change affect the source maps that are served as well?

jargv · 2016-03-23T16:52:57Z

@shurcooL Source maps seem to work as expected with this change.

dominikh · 2016-03-23T22:00:39Z

README.md

@@ -35,6 +35,8 @@ For example, navigating to `http://localhost:8080/example.com/user/project/` sho

 Refreshing in the browser will rebuild the served files if needed. Compilation errors will be displayed in terminal, and in browser console. Additionally, it will serve $GOROOT and $GOPATH for sourcemaps.

+If you include an argument, it will be the root from which everything is served. For example, if you run `gopherjs serve example.com/user` then the generated JavaScript will be served at `http://localhost:7979/project/project.js`.


I don't understand how the command gopherjs serve example.com/user maps to something being served at /project/.

I also feel like making it a positional argument is confusing. Other tools (build, install) take the package as an argument. I can accept serve not accepting a package, but IMO it shouldn't interpret the argument in an entirely different way. Wouldn't it make more sense to make this an explicit flag?

What about changing it to this:

"For example, if you run gopherjs serve github.com/user/project then the generated JavaScript for the package github.com/user/project/mypkg will be served at http://localhost:7979/mypkg/mypkg.js."

I think the semantics are not too far off compared to other commands.

I guess I don't understand what the argument does. I thought gopherjs serve foo would serve the package in $PWD at /foo/.

@dominikh, no, it does what @neelance described more clearly above. See #384 that this PR resolves for more context.

Hm, okay. I guess it's fine then.

This does behave like other commands which take a package. It just also handles the case where the argument is not a complete package path, in which case it serves all packages at that root.

neelance · 2016-03-25T10:34:44Z

README.md

@@ -35,6 +35,8 @@ For example, navigating to `http://localhost:8080/example.com/user/project/` sho

 Refreshing in the browser will rebuild the served files if needed. Compilation errors will be displayed in terminal, and in browser console. Additionally, it will serve $GOROOT and $GOPATH for sourcemaps.

+If you include an argument, it will be the root from which everything is served. For example, if you run gopherjs serve github.com/user/project then the generated JavaScript for the package github.com/user/project/mypkg will be served at http://localhost:7979/mypkg/mypkg.js.


One last thing: Let's use :8080 here, since that is the default.

neelance · 2016-03-25T10:35:38Z

I'd like the port number changed, see above. LGTM otherwise. @dominikh? @shurcooL?

dominikh · 2016-03-25T10:40:20Z

LGTM

dmitshur · 2016-03-26T03:01:04Z

I can see some minor followup enhancements, but no blockers for this getting merged to resolve #384.

LGTM.

jargv · 2016-03-26T04:47:22Z

I found an issue with sourcemaps that needs to be resolved before this can be merged.

The source maps work well enough to map errors to the right file/line. So for example instead of the error being reported at output.js:15912 it is reported correctly at main.go:7.

The problem occurs when I click on main.go:7, the devtools can't load main.go to display the error in the context of the file. I hope to have this fixed soon.

jargv · 2016-03-26T05:55:49Z

I added a fix but it has an assumption. When the requested file ends in ".go" I ignore the serveRoot. The assumption is that source maps will only map to .go files, and .go files will only be requested because the source maps reference them. Is that true?

flimzy · 2016-03-26T09:38:38Z

GopherJS also accepts *.inc.js files as input. I don't know if these are presently supported for source maps. If it's easy to support such files as well, I would encourage doing so. If that's presently not supported, then at least it's not a regression not to support them here.

jargv · 2016-03-26T13:59:56Z

@flimzy source maps don't appear to work for .inc.js files.

neelance · 2016-03-27T11:41:08Z

What about trying both? First try the prefixed path, but if that file does not exist then try the non-prefixed path? I think this should be possible by adding to serveCommandFileSystem.dirs. And also for the implementation in general, it might be easier to prefix entries of dirs instead of adding serveRoot. I just realized that now, sorry for not mentioning it earlier.

jargv · 2016-03-27T13:39:35Z

@neelance I actually tried that first. The trouble I ran into was how serveCommandFileSystem.Open determines when to compile or not. Specifically these variables never evaluated to true:

isPkg := file == base+".js"
isMap := file == base+".js.map"

because base is derived from the request path.

Of course this could be changed to work in some other way, but it's not clear to me that that would be simpler. For example, how would you disambiguate a request to "/"? Should that compile the package at serveRoot, or serve the directory listing with http.Dir(...).Open(...)?

That said, I'm happy to make the change if that's determined to be the best way forward.

…maps

jargv · 2016-03-29T22:01:56Z

Ok I changed it to serve files both inside and outside of the serveRoot. There's no longer special casing around .go files.

@neelance is this what you meant? I tried adding to serveCommandFileSystem.dirs in order to get rid of the serveRoot variable, but the problem becomes mapping back to a valid package path. Right now we can assume that the request path (with the serve root prepended) is a valid package without having to look through GOPATH and GOROOT looking for a match.

Anything else to make this good to merge?

dmitshur reviewed Mar 23, 2016
View reviewed changes

dominikh reviewed Mar 23, 2016
View reviewed changes

jargv force-pushed the master branch from 56de34e to 12f4dee Compare March 24, 2016 03:32

neelance reviewed Mar 25, 2016
View reviewed changes

jargv force-pushed the master branch from 12f4dee to 506f096 Compare March 25, 2016 15:39

dmitshur mentioned this pull request Mar 25, 2016

Review latest changes to https://github.com/gopherjs/gopherjs/pull/429. shurcooL-legacy/backlog#13

Closed

jargv added 2 commits March 25, 2016 22:55

Add optional root argument to gopherjs serve. Fixes gopherjs#384

af6637d

Fix serving go files (referenced by source maps)

dd20e3f

jargv force-pushed the master branch from 506f096 to dd20e3f Compare March 26, 2016 05:40

Serve files outside of serveRoot to handle files referenced by source…

fe61552

…maps

neelance merged commit 234e9ee into gopherjs:master Mar 31, 2016

		@@ -35,6 +35,8 @@ For example, navigating to `http://localhost:8080/example.com/user/project/` sho

		Refreshing in the browser will rebuild the served files if needed. Compilation errors will be displayed in terminal, and in browser console. Additionally, it will serve $GOROOT and $GOPATH for sourcemaps.

		If you include an argument, it will be the root from which everything is served. For example, if you run `gopherjs serve example.com/user` then the generated JavaScript will be served at `http://localhost:7979/project/project.js`.

Add optional root argument to gopherjs serve. Fixes #384 #429

Add optional root argument to gopherjs serve. Fixes #384 #429

Uh oh!

Conversation

jargv commented Mar 22, 2016

Uh oh!

neelance commented Mar 22, 2016

Uh oh!

dmitshur commented Mar 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Mar 23, 2016

Uh oh!

jargv commented Mar 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neelance commented Mar 25, 2016

Uh oh!

dominikh commented Mar 25, 2016

Uh oh!

dmitshur commented Mar 26, 2016

Uh oh!

jargv commented Mar 26, 2016

Uh oh!

jargv commented Mar 26, 2016

Uh oh!

flimzy commented Mar 26, 2016

Uh oh!

jargv commented Mar 26, 2016

Uh oh!

neelance commented Mar 27, 2016

Uh oh!

jargv commented Mar 27, 2016

Uh oh!

jargv commented Mar 29, 2016

Uh oh!

Uh oh!