javascript-externs-generator icon indicating copy to clipboard operation
javascript-externs-generator copied to clipboard

Incomplete externs for Slate: Missing non-enumerable and inherited properties

Open kommen opened this issue 7 years ago • 6 comments

I'm trying to generate externs for https://github.com/ianstormtaylor/slate, but there are a couple of extern declarations missing.

To reproduce:

  • go to https://jmmk.github.io/javascript-externs-generator/
  • load https://cdnjs.cloudflare.com/ajax/libs/react/15.3.2/react.js
  • load https://cdnjs.cloudflare.com/ajax/libs/react/15.3.2/react-dom.js
  • load https://cdnjs.cloudflare.com/ajax/libs/immutable/3.8.1/immutable.js
  • load https://unpkg.com/[email protected]/dist/slate.js
  • generate extern for the Slate namespace

This generates externs, including "Document": function () {}, which for example is missing its nodes property (see Slate docs) .

Am I missing something? Thanks in advance!

kommen avatar Oct 03 '16 09:10 kommen

Out of curiosity, what browser are you using? I've run into some strange issues in Chrome trying to debug this.

I think it doesn't get the nodes property because I'm iterating over Slate.Document rather than an instance of Document e.g. var doc = new Slate.Document(); for (var _prop in doc) { ... };

I'm not sure how the Closure Compiler would handle code using doc.nodes, or if the extern would fix it. There's a longstanding need to create some scripts to test Closure Compiler behavior for things like this.

Can you verify that the simple generated externs do or do not work when using instance properties (doc.nodes)?

jmmk avatar Oct 05 '16 11:10 jmmk

Using Chrome Version 54.0.2840.41 beta (64-bit). I ran into weird issues when trying the generator in Safari, where I can't even type into the URL field.

Can you verify that the simple generated externs do or do not work when using instance properties (doc.nodes)?

They externs don't work. When I add the nodes property manually to the generated externs they work.

kommen avatar Oct 05 '16 13:10 kommen

@kommen where did you add nodes? Can you paste the extern (or at least the section for Document?

jmmk avatar Oct 19 '16 22:10 jmmk

@jmmk: I replaced the line "Document": function () {}, (line 11) with

  "Document": {
    "nodes": {}
  },

See this gist: https://gist.github.com/kommen/becca5c08fe08bf2aca664320cc5ec78/revisions

This only fixes only the first of the issues with advanced optimizations, so there's a bunch of other externs missing still after adding nodes to Document.

kommen avatar Nov 02 '16 16:11 kommen

It seems there's a couples things going on here:

  1. Non-enumerable properties: Some properties are explicitly defined as non-enumerable, so for (var key in obj) won't pick them up, but Object.getOwnPropertyNames will.
  2. Inherited properties: Not only are some properties non-enumerable, but they are inherited, which means that Object.getOwnPropertyNames does not find them either.

To give a concrete example:

  • Slate.Document inherits an Immutable.OrderedMap that contains the key nodes.
  • Object.getOwnPropertyNames(Slate.Document.prototype) does not include nodes
  • Object.getOwnPropertyNames(Object.getPrototypeOf(Slate.Document)) does not include nodes
  • Object.getOwnPropertyNames(Object.getPrototypeOf(Slate.Document).prototype) does include nodes

I don't think my understanding of this is clear enough at the moment to make any changes, but it does seem possible to handle this scenario.

Reference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Enumerability_and_ownership_of_properties

jmmk avatar Apr 08 '17 14:04 jmmk

Some other notes:

  • goog.object has a function getAllPropertyNames that enumerates properties/methods up the prototype chain
  • At the very least, we should be including non-enumerable properties with Object.getOwnPropertyNames
  • If we do recurse with Object.getPrototypeOf, how do we determine what to put under the prototype vs just as a property? Does it matter since we are not defining constructors and inheritance - do we even need to specify that functions are part of the prototype rather than just properties of an object? May require testing in #22
  • Some explanations about constructors, .prototype and Object.getPrototypeOf: http://stackoverflow.com/questions/38740610/object-getprototypeof-vs-prototype

jmmk avatar Apr 09 '17 00:04 jmmk