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

Closed
lcodes opened this Issue Jun 15, 2016 · 5 comments

Comments

Projects
None yet
4 participants
@lcodes

lcodes commented Jun 15, 2016

Hello,

I get a crash when using the following:

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

However, this works fine:

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

I just tried with the d0e0934 version of esdoc and it still crashes, here is the stack trace:

/mnt/data/Projects/esdoc/src/Publisher/Builder/DocResolver.js:121
          if (autoPrivate && this.name.charAt(0) === '_') {
                                      ^

TypeError: Cannot read property 'charAt' of undefined
    at Object.<anonymous> (/mnt/data/Projects/esdoc/src/Publisher/Builder/DocResolver.js:68:37)
    at /mnt/data/Projects/esdoc/node_modules/taffydb/taffy.js:743:17
    at each (/mnt/data/Projects/esdoc/node_modules/taffydb/taffy.js:126:17)
    at Object.<anonymous> (/mnt/data/Projects/esdoc/node_modules/taffydb/taffy.js:740:7)
    at Object.API.(anonymous function) [as update] (/mnt/data/Projects/esdoc/node_modules/taffydb/taffy.js:166:18)
    at DocResolver._resolveAccess (/mnt/data/Projects/esdoc/src/Publisher/Builder/DocResolver.js:66:18)
    at DocResolver.resolve (/mnt/data/Projects/esdoc/src/Publisher/Builder/DocResolver.js:24:10)
    at CoverageBuilder.DocBuilder (/mnt/data/Projects/esdoc/src/Publisher/Builder/DocBuilder.js:21:27)
    at new CoverageBuilder (/mnt/data/Projects/esdoc/src/Publisher/Builder/CoverageBuilder.js:7:37)
    at publish (/mnt/data/Projects/esdoc/src/Publisher/publish.js:79:5)
child_process.js:529
    throw err;
    ^
@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Jul 13, 2016

Aye, we hit this today as well. @JoelEinbinder

Aye, we hit this today as well. @JoelEinbinder

typhonrt added a commit to typhonjs-node-esdoc/esdoc that referenced this issue Jul 14, 2016

@typhonrt

This comment has been minimized.

Show comment
Hide comment
@typhonrt

typhonrt Jul 14, 2016

Contributor

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'; }
}
Contributor

typhonrt commented Jul 14, 2016

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'; }
}
@h13i32maru

This comment has been minimized.

Show comment
Hide comment
@h13i32maru

h13i32maru Jul 18, 2016

Member

@ddude Thanks for report this bug. I fixed it on current master. I will publish new version at shortly.
Thanks.

Member

h13i32maru commented Jul 18, 2016

@ddude Thanks for report this bug. I fixed it on current master. I will publish new version at shortly.
Thanks.

@typhonrt

This comment has been minimized.

Show comment
Hide comment
@typhonrt

typhonrt Jul 18, 2016

Contributor

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.

Contributor

typhonrt commented Jul 18, 2016

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.

h13i32maru added a commit that referenced this issue Dec 31, 2016

@h13i32maru

This comment has been minimized.

Show comment
Hide comment
@h13i32maru

h13i32maru Dec 31, 2016

Member

I fixed this issue in e59820a
And I will release in next version

Member

h13i32maru commented Dec 31, 2016

I fixed this issue in e59820a
And I will release in next version

@h13i32maru h13i32maru closed this Dec 31, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment