Crash when using x[Symbol.toStringTag] = ... #297

Activity

1. paulirish commented on Jul 13, 2016

   

   paulirish

   on Jul 13, 2016

   More actions

   Aye, we hit this today as well. @JoelEinbinder
2. typhonrt commented on Jul 14, 2016

   

   typhonrt

   on Jul 14, 2016

   Contributor

   More actions

   Hi folks... So the reason for ESDoc failing here is that it is only assuming Identifier AST nodes. to determine the name. When doing the following this makes toStringTag an Identifier node:

   let toStringTag = Symbol.toStringTag;
   immutable.Map.prototype[toStringTag] = function _toStringTag() {
     return 'Map';
   };
   

   I have a separate fork of ESDoc that supports Babylon as the parser and I have made a patch that fixes this problem, but I want to get your guys feedback before submitting a PR to ESDoc.

   I also added support for MyClass.prototype['ABC'] = ... or accepting a Literal / StringLiteral AST node to define the name.

   It should be noted that currently ESDoc (or my fork) doesn't parse adding methods to the prototype of a class. IE MyClass.prototype.newMethod = () => { } is not added as a method in the class docs, but ends up as an unexported function doc. So you'll see it if you set "unexportIdentifier": true in your ESDoc configuration file, but it comes up as a function separate of the class.

   I'd like to receive some comments on this as a potential enhancement such that when adding methods to a class prototype should they be picked up in the class documentation as a class method. It requires a bit more effort to support this with ESDoc, but should be possible.

   For now though could you guys take a look and give a quick test with:
   https://github.com/typhonjs-node-esdoc/esdoc

   It's not published on NPM so you have to include it directly from GitHub ala:

   devDependencies: {
     "esdoc": "git+https://git@github.com/typhonjs-node-esdoc/esdoc.git"
   }
   

   ---

   A secondary option if you have full control of the source code and want to see toStringTag to show up in the class method docs is this:

   export default class MyClass
   {
     /**
      * Defines toStringTag. 
      * @returns {string}
      */
      [Symbol.toStringTag]() { return 'MyClass'; }
   }
   
3. h13i32maru

   added a commit that references this issue on Jul 18, 2016

   fix(function): crash when function name has member expression

   ...

   624975a
4. h13i32maru

   added

   Result: Accept

   Type: Bug

   Scope: Internal

   on Jul 18, 2016
5. h13i32maru commented on Jul 18, 2016

   

   h13i32maru

   on Jul 18, 2016

   Member

   More actions

   @ddude Thanks for report this bug. I fixed it on current master. I will publish new version at shortly.
   Thanks.
6. typhonrt commented on Jul 18, 2016

   

   typhonrt

   on Jul 18, 2016

   Contributor

   More actions

   You might also handle the literal case:

         if (this._node.id.type === 'MemberExpression') {
           this._value.name = ASTUtil.flattenMemberExpression(this._node.id);
         } 
         else if (this._node.id.type === 'Literal') {
           this._value.name = this._node.id.value;
         } else {
           this._value.name = this._node.id.name;
         }
   

   Without supporting the Literal ESTree AST Node here this will crash: MyClass.properties['something'] = function() { ... } will crash just like MemberExpression did previously.
7. h13i32maru

   added a commit that references this issue on Dec 31, 2016

   fix(function): crash when function is assigned with member expression

   ...

   e59820a
8. h13i32maru commented on Dec 31, 2016

   

   h13i32maru

   on Dec 31, 2016

   Member

   More actions

   I fixed this issue in e59820a
   And I will release in next version
9. h13i32maru

   closed this as completedon Dec 31, 2016
10. h13i32maru

    added this to the next-version milestone on Dec 31, 2016