infer displayName when not defined #59

NogsMPLS · 2016-03-01T22:50:15Z

For helping out with #30.

Added tests and passes tests on my end for es6 class with no static displayName attached and also stateless component.

Would like a second pair of eyes in src/handlers/displayNameHandler.js on line 34. I'm not sure if I like that solution using [0], but not sure how to test around it.

NogsMPLS · 2016-03-01T22:50:55Z

src/handlers/displayNameHandler.js

+    }
+    displayNamePath = displayNamePath.node.value
+  } else if (!displayNamePath) {
+    displayNamePath = path.node.id ? path.node.id.name : path.node.declarations[0].id.name;


Would like a second pair of eyes on this, specifically around path.node.declarations[0].id.name. I'm not sure if I like it.

See my comment above :) (which should make this a non-issue)

fkling · 2016-03-02T16:30:25Z

Thanks for working on this! I will add some comments inline.

fkling · 2016-03-02T16:40:02Z

src/handlers/__tests__/displayNameHandler-test.js

+    expect(documentation.displayName).toBe('FooBar');
+
+    definition = statement(`
+      var Foo = () => {


Unfortunately this is not useful test, because the displayNameHandler function will never be passed a VariableDeclaration.

If you look at getMemberValuePath, you can see the node types which represent component definitions:

var LOOKUP_METHOD = { [types.ArrowFunctionExpression.name]: getMemberExpressionValuePath, [types.FunctionExpression.name]: getMemberExpressionValuePath, [types.FunctionDeclaration.name]: getMemberExpressionValuePath, [types.VariableDeclaration.name]: getMemberExpressionValuePath, [types.ObjectExpression.name]: getPropertyValuePath, [types.ClassDeclaration.name]: getClassMemberValuePath, [types.ClassExpression.name]: getClassMemberValuePath, };

For some of them, inferring the name is easy: You can simple take the id of named classes and functions.

For all others, you need to traverse up the tree to find and assignment or export and inspect that one to get the name.

Here is how getMemberExpressionValuePath does it:

react-docgen/src/utils/getMemberExpressionValuePath.js

Lines 36 to 45 in 9042c91

if (

types.FunctionExpression.check(path.node) ||

types.ArrowFunctionExpression.check(path.node)

) {

if (!types.VariableDeclarator.check(path.parent.node)) {

return; // eslint-disable-line consistent-return

}

return path.parent.get('id', 'name').value;

}

Okay so, I still want to be able to infer the name and assign them as the displayName of stateless functional components.

What would you think about exporting resolveName() from /getMemberExpressionValuePath.js, importing it into displayNameHandler and using that to grab the variable declaration display names?

NogsMPLS · 2016-03-02T17:34:49Z

blah, fail test. ill check.

edit: ah, okay, test is doing commonjs require, so really just wants one export per file. Makes sense.

Would breaking resolveName into separate file be cool?

edit2: I can add some tests around this too if this is considered an okay approach

fkling · 2016-03-02T18:04:30Z

Would breaking resolveName into separate file be cool?

Sure! Can you also update the tests to verify whether these cases are covered?

Function expression: (function Component() {})
Named exports:
- export var Component = function() {}
- exports.Component = function() {}

It was also be great to get some test coverage for React.class(...) component definitions. This might require some changes to the way the tests are set up. Have a look at https://github.com/reactjs/react-docgen/blob/master/src/handlers/__tests__/componentDocblockHandler-test.js for how different component definitions could be tested easily.

NogsMPLS · 2016-03-02T20:46:42Z

You're talking about mainly updating the displayNameHandler-test.js to handle those use cases right?

fkling · 2016-03-02T21:48:50Z

Yes.

NogsMPLS · 2016-03-02T23:17:15Z

Almost got this, named exports giving me a little trouble, but I think I should have something tomorrow.

NogsMPLS · 2016-03-03T16:50:48Z

The biggest issue I'm running into with Named Exports has to do with Node vs. NodePath.

Basically, inside getMemberValuePath.js I can get the declaration of the export (i.e. VariableDeclartion/ClassDeclaration/FunctionDeclaration) by doing something like:

//checks if named export, then redefines the componentDefinition to be the export's declaration
  if (componentDefinition.node.type === types.ExportNamedDeclaration.name) {
    componentDefinition = componentDefinition.get('declaration').value;
}

However, once I grab it and redefine componentDefinition to the new declaration, componentDefinition stops being a NodePath and starts being a Node, which screws with about 4 other files that end up using path.node, which, as far as I can tell, only exists for NodePath type.

I think I can make it work all around, but it's gonna be a bigger refactor than just 1 file.

fkling · 2016-03-03T17:13:27Z

After looking at the AST, it seems named exports should actually be covered by variable declarations, no?

Regarding exports.name = ... it seems you would need to look whether the parent is an assignment expression to exports.X and grab the property name.

getMemberValuePath is for getting keys from the component definition (i.e. propTypes, defaultProps, etc), it wouldn't help you finding information "outside" the component definition.

If you have something new to share, I'm happy to take a look.

NogsMPLS · 2016-03-03T17:46:00Z

Oh neat, didn't know that ast tool existed. (I'm still pretty new to parsing the AST)

well, so, first thing I did was just add a test to displayNamedHandler-test.js, that was simply:

it('infers the displayName with named export', () => {
    var definition = statement(`
      export var Foo = function() {}
    `);
    displayNameHandler(documentation, definition);
    expect(documentation.displayName).toBe('Foo');
  });

But of course, that throws an error because the definition type is ExportNamedDeclaration, which getmemberValuePath.js doesn't like.

So my line of reasoning was to go one level deeper into the ExportNamedDeclaration AST object where things like VariableDeclaration are defined and use that, but I'm ran into path.node not existing issues.

NogsMPLS · 2016-03-03T22:27:21Z

i expect this latest to trip travis-ci. just pushing so i have it up there for now.

NogsMPLS · 2016-03-04T20:52:44Z

Was able to get infer name on es6 exports to work on classes, function expressions and variable declarations. (though I had to do some stuff that feels very clunky for variable declaration)

now to get exports.name working and we should be there

NogsMPLS · 2016-03-04T21:57:16Z

Got a version of infering displayName on commonJS exports working.

Not sure if I like the solution right now.

Need to add more tests around exports and attaching displayName vs. inferring it and make sure the more explicit attaching of displayName takes precedent.

NogsMPLS · 2016-03-05T21:07:30Z

src/handlers/displayNameHandler.js

+
+    } else if (declaration.type === types.VariableDeclaration.name) {
+
+      declarationPath = resolveExportDeclaration(path)[0].parentPath.parentPath.parentPath;


I don't really like this solution, but I'm not sure of how else to get this value while keeping 'NodePath' type intact.

NogsMPLS · 2016-03-05T21:11:52Z

Looking at stuff over again today, I decided it's probably best to keep current behavior in regards to an example like:

export class Foo {
   static displayName = foo.bar;
}

Initially, I was going to try and have it infer the class name as the displayName in that case, but I think the better behavior might be to keep it empty returning as it currently is.

All tests are passing, would like a review, especially on how I'm handling immediately exported variable declarations.

Other than that though, I think this satisfies #30 as far as I can tell.

fkling · 2016-03-12T17:40:24Z

src/handlers/__tests__/displayNameHandler-test.js

+
+});
+
+describe('defaultPropsHandler with ES6 Exports', () => {


I guess this should be "dispayNameHandler".

fkling · 2016-03-12T17:55:44Z

I'll take a closer look again today, thanks!

NogsMPLS · 2016-07-28T21:22:15Z

hmmm....tests passed on stable and node 5 but failed on node 4

It passes locally for me on Node 4.

CI looks like it's failing on a file this pull request doesn't touch. Seems like it's maybe a fluke fail?

NogsMPLS · 2016-07-29T14:24:50Z

looks like this is what is keeping #97 from passing as well.

I did a bit of digging and found a jest issue mentioning this:

jestjs/jest#595

This seems to be an intermittent fail from what I can tell.

perhaps updating jest from ^12.1.1 to ^14.0.1 might fix, but going up 2 majors should probably be its own pull request.

If you think getting jest updated is important, I can work and get a PR in for that.

fkling · 2016-08-02T15:38:51Z

We can at least give it a try yes!

NogsMPLS · 2016-08-08T14:09:35Z

Oh also, I 100% completely understand if this branch/feature isn't something you'd want merged into the main trunk. It solves a problem for my purposes though, so I've been linking to a fork of this for my projects.

Along those lines, I'd like to ask your permission for me to create a npm package with this feature included in it so I can stop linking directly to github in a few of my projects.

ZauberNerd · 2016-08-15T18:02:29Z

@NogsMPLS I'm also very interested in using this. If it is not going into upstream, could you create a separate npm package for this handler?
I documented a little bit how I created a custom handler in this issue: #115
As an example you could take a look at these two packages: nerdlabs/react-docgen-imports-handler and nerdlabs/react-docgen-exports-handler
As Felix seems to be very busy at the moment (see linked issue above) I'd probably just go ahead and create an npm package for this. And if this should be in upstream at a later stage you could still open a new PR and deprecate your package?

NogsMPLS · 2016-08-15T23:43:57Z

Ah, interesting, I didn't see that post about future of react-docgen.

yeah I can look into making this into a custom handler.

fkling · 2016-08-18T15:17:29Z

I'd probably just go ahead and create an npm package for this.

yeah, that would be the best solution for now. I'm happy to merge any fixes that would need to be done to core utilities though.

ZauberNerd · 2016-09-09T19:50:01Z

@NogsMPLS So, I've had some spare time and went ahead and created a displayName handler. Would that be covering all of your use cases too? https://github.com/nerdlabs/react-docgen-displayname-handler

NogsMPLS · 2016-09-10T18:09:27Z

@ZauberNerd nice! Ill check it out sometime this week and let you know

danny-andrews · 2017-04-04T20:42:09Z

Is this PR died? It would be really nice to have this feature! @NogsMPLS

NogsMPLS · 2017-04-04T20:46:47Z

@danny-andrews I believe the display-name-handler that @ZauberNerd created does a decent job of covering what this PR was meant to do.

I know of at least 1 pretty popular repo that is using that display-name-handler plugin: https://github.com/styleguidist/react-styleguidist

I am going to go ahead and close this PR with the advice that people look into using that plugin instead of anything in this PR specifically.

sapegin · 2017-04-05T06:38:28Z

Yeah, we’re using display-name-handler in React Styleguidist and it works perfectly!

danny-andrews · 2017-04-05T15:56:16Z

Thanks!

NogsMPLS added 3 commits March 1, 2016 15:34

component name fallback implementation

b14d938

more defensive conditional

f1487ab

infer stateless component displayName

45c84f2

NogsMPLS reviewed Mar 1, 2016
View reviewed changes

removing duplicate test

7880236

fkling reviewed Mar 2, 2016
View reviewed changes

use resolveName() for functional components

cd711b2

turn resolveName into separate module

32636c5

adding failing test

caa435a

NogsMPLS added 2 commits March 4, 2016 14:48

Immediately Exported ES6 Modules passing tests

09d541c

fix extract displayname test

59e4f9c

NogsMPLS added 2 commits March 4, 2016 15:52

Merge remote-tracking branch 'upstream/master' into inferDisplayName

437a123

infer displayName on immediately exported commonJS exports

05e4e67

add test, remove unnecessary import

588daa5

NogsMPLS reviewed Mar 5, 2016
View reviewed changes

fkling reviewed Mar 12, 2016
View reviewed changes

name tests correctly

bab57a6

ghost added the CLA Signed label May 19, 2016

NogsMPLS added 2 commits July 28, 2016 15:54

Merge remote-tracking branch 'upstream/master' into inferDisplayName

60dfe63

fix displayNameHandler tests

5e684de

NogsMPLS mentioned this pull request Aug 2, 2016

update jest to 14 #104

Closed

ghost added the CLA Signed label Aug 15, 2016

ghost added the CLA Signed label Sep 9, 2016

ghost added the CLA Signed label Sep 10, 2016

NogsMPLS mentioned this pull request Feb 3, 2017

Npm install fails when run with cygwin. DigitalRiver/react-atlas#100

Closed

NogsMPLS closed this Apr 4, 2017

github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

infer displayName when not defined #59

infer displayName when not defined #59

NogsMPLS commented Mar 1, 2016

NogsMPLS Mar 1, 2016

fkling Mar 2, 2016

fkling commented Mar 2, 2016

fkling Mar 2, 2016

NogsMPLS Mar 2, 2016

NogsMPLS commented Mar 2, 2016

fkling commented Mar 2, 2016

NogsMPLS commented Mar 2, 2016

fkling commented Mar 2, 2016

NogsMPLS commented Mar 2, 2016

NogsMPLS commented Mar 3, 2016

fkling commented Mar 3, 2016

NogsMPLS commented Mar 3, 2016

NogsMPLS commented Mar 3, 2016

NogsMPLS commented Mar 4, 2016

NogsMPLS commented Mar 4, 2016

NogsMPLS Mar 5, 2016

NogsMPLS commented Mar 5, 2016

fkling Mar 12, 2016

fkling commented Mar 12, 2016

NogsMPLS commented Jul 28, 2016 •

edited

Loading

NogsMPLS commented Jul 29, 2016

fkling commented Aug 2, 2016

NogsMPLS commented Aug 8, 2016

ZauberNerd commented Aug 15, 2016

NogsMPLS commented Aug 15, 2016

fkling commented Aug 18, 2016

ZauberNerd commented Sep 9, 2016

NogsMPLS commented Sep 10, 2016

danny-andrews commented Apr 4, 2017 •

edited

Loading

NogsMPLS commented Apr 4, 2017

sapegin commented Apr 5, 2017

danny-andrews commented Apr 5, 2017

	if (
	types.FunctionExpression.check(path.node) \|\|
	types.ArrowFunctionExpression.check(path.node)
	) {
	if (!types.VariableDeclarator.check(path.parent.node)) {
	return; // eslint-disable-line consistent-return
	}

	return path.parent.get('id', 'name').value;
	}


		} else if (declaration.type === types.VariableDeclaration.name) {

		declarationPath = resolveExportDeclaration(path)[0].parentPath.parentPath.parentPath;

infer displayName when not defined #59

infer displayName when not defined #59

Conversation

NogsMPLS commented Mar 1, 2016

NogsMPLS Mar 1, 2016

Choose a reason for hiding this comment

fkling Mar 2, 2016

Choose a reason for hiding this comment

fkling commented Mar 2, 2016

fkling Mar 2, 2016

Choose a reason for hiding this comment

NogsMPLS Mar 2, 2016

Choose a reason for hiding this comment

NogsMPLS commented Mar 2, 2016

fkling commented Mar 2, 2016

NogsMPLS commented Mar 2, 2016

fkling commented Mar 2, 2016

NogsMPLS commented Mar 2, 2016

NogsMPLS commented Mar 3, 2016

fkling commented Mar 3, 2016

NogsMPLS commented Mar 3, 2016

NogsMPLS commented Mar 3, 2016

NogsMPLS commented Mar 4, 2016

NogsMPLS commented Mar 4, 2016

NogsMPLS Mar 5, 2016

Choose a reason for hiding this comment

NogsMPLS commented Mar 5, 2016

fkling Mar 12, 2016

Choose a reason for hiding this comment

fkling commented Mar 12, 2016

NogsMPLS commented Jul 28, 2016 • edited Loading

NogsMPLS commented Jul 29, 2016

fkling commented Aug 2, 2016

NogsMPLS commented Aug 8, 2016

ZauberNerd commented Aug 15, 2016

NogsMPLS commented Aug 15, 2016

fkling commented Aug 18, 2016

ZauberNerd commented Sep 9, 2016

NogsMPLS commented Sep 10, 2016

danny-andrews commented Apr 4, 2017 • edited Loading

NogsMPLS commented Apr 4, 2017

sapegin commented Apr 5, 2017

danny-andrews commented Apr 5, 2017

NogsMPLS commented Jul 28, 2016 •

edited

Loading

danny-andrews commented Apr 4, 2017 •

edited

Loading