Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open() unused? #4

Closed
ben-clayton opened this issue Jun 2, 2015 · 6 comments
Closed

Open() unused? #4

ben-clayton opened this issue Jun 2, 2015 · 6 comments

Comments

@ben-clayton
Copy link

It appears that Open is unused by the package. I'm guessing you added it for font loading before they were embedded?
If it isn't required, removing it would also kill the dependency on "honnef.co/go/js/xhr".

@dmitshur
Copy link
Member

dmitshur commented Jun 4, 2015

Hi @ben-clayton,

There are multiple ways to resolve this issue. Before picking one, let me explain a few things and suggest alternatives, and then I will ask for your additional thoughts. I don't know how familiar you are with some things, so I will explain them. Sorry if you already know this.

Short Summary

Yes, glfw.Open is currently unused by gxui, but it is required. It can either be factored out into a separate package, or rewritten not to import "honnef.co/go/js/xhr", or be left as is.

Full Details

First, you are right, glfw.Open is unused in my gxui PR. It was originally used for font loading before they were embedded, that is correct.

However, it was added prior to that. It is used in multiple other places where I do need to load external resources. For example, see:

It's needed to load external resources because os.Open is not available on all platforms, but it abstracts out that platform difference (on the web, it is implemented by making an XHR request to backend server which is expected to serve the requested resource).

However, a question is whether it should be a part of glfw library or factored out into a separate library. Traditionally, GLFW has two core responsibilities: it is responsible for creating a window and a GL context, and it is responsible for providing user input (mouse, keyboard, touch, joysticks, pen and tablets, etc.). It hides the platform differences and provides a uniform API to do those two tasks. In this way, Open feels like I'm adding a third responsibility: providing a unified interface for loading external assets. I am unsure if it should be a part of GLFW or factored out. Keeping it inside is helpful because all browser-based apps will need Open to load assets. But perhaps moving it outside is better, since it can be done and then there can be multiple implementations. Thoughts?


Ok, a separate question is about the actual implementation of Open.

If it isn't required, removing it would also kill the dependency on "honnef.co/go/js/xhr".

First, are you aware that the "honnef.co/go/js/xhr" dependency is "hidden" behind the js build tag, meaning that it's not a dependency on other platforms? It is not fetched if one does go get github.com/goxjs/glfw. I just want to find out if you already know this.

Next, if you are doing go get -tags=js github.com/goxjs/glfw and plan to use this library with GopherJS compiler to compile for the browser, then... Do you already know that "honnef.co/go/js/xhr" is considered the canonical Go binding package for XHR API? See here. Therefore, it is a very commonly used package in this context.

Finally, as of commit gopherjs/gopherjs@fdbe407, GopherJS compiler provides an XHRTransport-based implementation of http.DefaultClient. As a result, I can now re-write that code by using net/http package instead of using the XHR bindings. This would result in the "honnef.co/go/js/xhr" dependency in no longer being used even on GOARCH=js.

Do you have any additional thoughts or preferences on which of the above solutions I pick to resolve this issue? Thanks!

@ben-clayton
Copy link
Author

Keeping it inside is helpful because all browser-based apps will need Open to load assets. But perhaps moving it outside is better, since it can be done and then there can be multiple implementations. Thoughts?

Yes. I find it odd that your goxjs/glfw library, which by its name would suggest it's a JS/WebGL port of GLFW would have additional features. You already have goxjs/glfw and goxjs/gl, what about goxjs/io?

First, are you aware that the "honnef.co/go/js/xhr" dependency is "hidden" behind the js build tag, meaning that it's not a dependency on other platforms? It is not fetched if one does go get github.com/goxjs/glfw. I just want to find out if you already know this.

I am, but given how cool the WebGL stuff is, I'd like to promote it as much as the regular native renderer.

Do you already know that "honnef.co/go/js/xhr" is considered the canonical Go binding package for XHR API? See here. Therefore, it is a very commonly used package in this context.

I do, however I find it slightly strange that gopherjs hasn't adopted these libraries directly into their GitHub page.

My concerns are:

  • Increasing the number of dependencies, direct or transitive, increases the chance of something breaking. Unlike a breakage in GXUI which I can fix, other breakages are out of my hands, but that isn't always obvious to the person who has tried grabbing latest.
  • It was surprising to me to have to grab a 'xhr' project - I knew GXUI wasn't requiring file assets, I doubted your goxjs library required file assets, so I was actually suspicious to why it was there at all.
  • Grabbing a project from a personal domain was also unusual. I'm sure Mr Honnef is a very trustworthy guy, but I'm still uneasy about having GXUI depend on code being hosted by an individual. I see that all the source is mirrored on GitHub, but given we're pulling from "honnef.co/go/js/xhr", the actual source could easily be changed without anyone noticing. I'm also aware that we're depending on "honnef.co/go/js/dom", but I also know that it is required for your magic to work.
  • Using a project with so many dependencies can be off-putting to people evaluating a library, for all the reasons listed above.

@dominikh
Copy link

dominikh commented Jun 4, 2015

Mr Honnef here.

The code isn't mirrored on GitHub, it's actually the primary (and only) host. honnef.co/go/js/xhr is only a vanity import that directs go get to fetch the sources from GitHub. Yes, I could just change that and serve other code, but I could also replace the whole code if it were a github-based import, and most people would probably not notice for quite a while ;-)

As for why GopherJS hasn't "adopted" the library: Even though I'm part of the organization, I do consider those bindings my personal projects, and as such keep them under my own name. Same goes for my DOM bindings, which are also the canonical DOM bindings for GopherJS users.

(Yes, these arguments don't make me more trustworthy.)

Now, from a technical point of view, I do wonder if using my XHR package is actually required. As Dmitri pointed out, GopherJS's version of net/http has basic support for XHR nowadays. My XHR bindings are now only required when needing greater control over the transport, i.e. when one has to avoid the net/http abstraction.

@ben-clayton
Copy link
Author

I'm sorry @dominikh if I caused offence. I'm sure you'd never do anything underhand. I'm just being over cautious due to the fact GXUI is code owned by my employer, but maintained by me. That is a distiction that few would make if something unpleasant were to be executed on a few thousand machines.
You raise a good point though, in that I should treat personal domains as least as trustworthy as any other code-hosting site, github included.

I'll leave it to @shurcooL to decide what to do with the xhr dependency.

Cheers,
Ben

@dominikh
Copy link

dominikh commented Jun 4, 2015

No offence taken, it's a reasonable stance to have.

@dmitshur
Copy link
Member

dmitshur commented Jun 5, 2015

I'll leave it to @shurcooL to decide what to do with the xhr dependency.

I will replace it with net/http because that will work for my purposes. It didn't exist when I wrote that code, that's why it started off using js/xhr.

I find it odd that your goxjs/glfw library, which by its name would suggest it's a JS/WebGL port of GLFW would have additional features. You already have goxjs/glfw and goxjs/gl, what about goxjs/io?

GLFW stands for GL FrameWork library, and the 2.x version used to have a larger number of utilities, including funcs to load textures, timing funcs (these are still present), and even funcs to create threads, etc. Many of these auxiliary functions were originally helpful, later on they became less useful and redundant and out of scope. Fortunately, with GLFW 3.x the API was reduced to just the minimal core functionality (window/context creation and user input).

When I originally added glfw.Open, it was because I thought it was in line with providing utilities needed for most common tasks. When you're in a browser environment, loading resources cannot be easily done via os.Open, so I created this glfw.Open helper.

I think a better approach in the future will be to move glfw.Open out into a separate package like you suggested (and mention it in README or sample code, just to make sure it's discoverable enough). That will keep goxjs/glfw API smaller (same as the C library), and allow different implementations of Open to exist. I will think about that more and do this later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants