emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.74k stars 3.3k forks source link

WebIDL static on prototype vs. function/class definition is not correct #12676

Open netpoetica opened 3 years ago

netpoetica commented 3 years ago

In JavaScript, and really in all languages as I understand it, what static really means is that it is a member of the class definition rather than the instance.

In Emscripten's parsing of WebIDL, it takes these static methods and places them on the prototype. I am really not sure what the thinking was here, I would be curious to know, but in JS, this is how we think of static:

// WebIDL
interface Person {
  void Person();
  static long TotalPeople;
  static Person Create (DOMString name);
}
// C++ (ish, forgive me if not perfect)
struct Person
{
    static uint32_t TotalPeople = 0;
    std::string Name;
    Person()
    {
        TotalPeople += 1;
    }
   static Person Create(std::string name)
   {
      Person p;
      p.Name = name;
      return p;
   }
}
// JavaScript
function Person (){}
Person.prototype.Name = "";
Person.TotalPeople = 0;
Person.Create = function(name) {
   var p = new Person();
   p.Name = name;
   return p;
}

static is really just a property directly on the class (function).

IMHO it feels quite dirty accessing the prototype in this way, but I do stand ready to be slapped into rethinking my understanding of this, if necessary. However, I think the WebIDL spec is on my side here: https://heycam.github.io/webidl/#idl-static-attributes-and-operations

[Exposed=Window]
interface Circle {
  // ... omitted for brevity
  static Point triangulate(Circle c1, Circle c2, Circle c3);
};

  // ... omitted for brevity

typeof Circle.triangulate;            // Evaluates to "function"
Circle.prototype.triangulate;         // Evaluates to undefined

FWIW in my own local testing, I am not seeing static WebIDL definitions even show up on the prototype, which is what led me down this road. I haven't ruled out a user issue yet so this post isn't about that, but I am concerned about the emscripten interpretation of WebIDL static.

kripken commented 3 years ago

You may be right here. I think in the early work on the WebIDL binder I used static to implement C++ static, basically. Something that doesn't need an instance. I thought using the prototype was the best way to do it, but that might be wrong based on your last link.

Changing it now would be a breaking change, however, so the question is whether that's worth it. I'm not sure offhand.

netpoetica commented 3 years ago

I think one solution would be to attach it to the class definition (function) as well as the prototype, and mark the one on the prototype as deprecated with JSDoc (which most IDEs will popup nicely), for future deprecation. I would imagine that this is just a pointer to the C++ method, so it shouldn't be any significant overhead, no?

One thing I think we can say is the WebIDL doesn't seem to be highly used yet in the emscripten community, and I base this on the fact that I have been facing a major uphill battle trying to find any kind of tutorials or resources on the subject online. I do think, however, that it will become more popular because most JS folks who are serious enough to use C++ are probably keen on TypeScript, and generating types from WebIDL is much, much easier than parsing C++ files to generate types. Someone at https://github.com/charto/nbind seems to have done a great job of generating TypeScript d.ts type definition files directly from embind, but if I am being honest, I have been over their code multiple times and genuinely don't know how they are doing it :D

Anyhow, I think in order to support the growth of WebIDL, it would be best if emscripten complies with the W3 standard. I agree backwards compatibility is hugely important, so maybe until the next major release of emscripten, it could just live on both places, and then folks who are using the WebIDL spec (which is the second best resource available next to emscripten WebIDL documentation, sadly) will be able to rely on emscripten to be spec-compliant.

kripken commented 3 years ago

Adding it to both places would regress code size a little, but may be worth it, good idea.

One of the major users of the WebIDL binder is https://github.com/kripken/ammo.js . I think it would be a good idea to open an issue there for discussion with people using it in production.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.