doc(jsdoc): jsDoc for Component & connector documentation #1435

bobylito · 2016-10-18T16:56:19Z

First step for the way we manage the documentation of the components and the connectors. I'm sure we'll find lots of stuff to add :) (I need to rebase or something :) )

algobot · 2016-10-18T16:56:35Z

By analyzing the blame information on this pull request, we identified @Morhaus and @vvo to be potential reviewers

vvo · 2016-10-19T08:52:55Z

Some conflicts

vvo · 2016-10-19T09:13:38Z

docgen/plugins/navigation.js

+export default function() {
+  return function(files, metalsmith, done) {
+    const categories = {};
+    forEach(files, (data, path) => {


Minor comment: I tend to just use Object.entries(map, [path, data] => {}) (in other parts of plugins).

vvo · 2016-10-19T09:14:42Z

docgen/plugins/navigation.js

+      });
+    });
+
+    forEach(files, (data, path) => {


Maybe add a small comment on why do we need to loop twice?

vvo · 2016-10-19T09:15:33Z

docgen/plugins/onlyChanged.js

+  if (file.stats && file.stats.ctime) {
+    return Date.parse(file.stats.ctime) > lastRunTime || Date.parse(file.stats.mtime) > lastRunTime;
+  } else {
+    return true;


Maybe later in the file can we reuse hasChanged since it now handles the case where there's no stats?

Ok I fixed what went wrong here in the code that checks the availability of stats. I also kept a check on the value used anyway (boyscout rule cc @pixelastic)

vvo · 2016-10-19T09:16:28Z

docgen/src/Styling.md

-nav_groups:
-  - core
-nav_sort: 1
+category: core


So category: core means it's a guide? Maybe we could express this by using category: guide

Good point!

vvo · 2016-10-19T09:17:36Z

docgen/start.js

@@ -17,6 +17,7 @@ builder({middlewares}, err => {

 // then we watch and rebuild
 watch([
+  rootPath('packages/react-instantsearch/src/**/*.js'),


Do you remember why you had to add this line? The devServer is supposed to handle this part (watch webpack build, rebuild, reload)

Now there is content in the js files, that's why we should trigger the build of the jsdoc (which is in the metalsmith build)

Ah yes the jsDoc needs to be re-read, got it.

Previously this was handled by the markdown files updates

I guess we can remove the line watching markdown files then

vvo · 2016-10-19T09:18:13Z

package.json

    "lodash": "4.16.2",
    "marked": "0.3.6",
    "material-ui": "0.15.4",
-    "memoizee": "0.4.1",


I guess "react-docgen" is no more needed also

vvo · 2016-10-19T09:19:08Z

packages/react-instantsearch/src/core/InstantSearch.js

@@ -182,4 +191,31 @@ class InstantSearch extends Component {
  }
 }

+InstantSearch.propTypes = {


Is this needed because of jsdoc parsing limitation? (static vs .)

bobylito · 2016-10-19T09:26:04Z

Yes I know, I'm working on them :P

alexkirsz · 2016-10-19T12:38:11Z

packages/react-instantsearch/src/core/InstantSearch.js


-  onHistoryInternalStateUpdate = state => {
+  onHistoryInternalStateUpdate(state) {
    this.aisManager.onExternalStateUpdate(state);


How does that work? The state => notation automatically binds to the current instance. In that case, since the onHistoryInternalStateUpdate method is passed by reference, it looses its context, so we need to have it bound to the InstantSearch instance somehow.

It does not. :(

Fixed

alexkirsz · 2016-10-19T15:41:54Z

packages/react-instantsearch/src/core/InstantSearch.js

+      this._aisContextCache = {
+        ais: {
+          onInternalStateUpdate: this.onWidgetsInternalStateUpdate.bind(this),
+          createHrefForState: this.createHrefForState.bind(this),


Usually you'd just bind/reassign them in the constructor.

constructor(props) { /* ... */ this.onWidgetsInternalStateUpdate = this.onWidgetsInternalStateUpdate.bind(this); // etc. }

Is there a particular reason we want to avoid the method = () => construct?

Yes but bind is only mandatory because we export those. So this way makes the bind more explicit and localised. I didn't choose to do it the way you propose because I thought that it wasn't needed.

We avoid the assignment construct in class because jsdoc is as of now unable to parse it :(

Is there an issue on the jsdoc project speaking about this? I am curious.

vvo · 2016-10-19T19:30:12Z

package.json

@@ -56,6 +56,7 @@
    "google-map-react": "^0.20.0",
    "insert-css": "^1.0.0",
    "jest-cli": "^16.0.0",
+    "jsdoc-parse": "^1.2.7",


Could we use the latest jsdoc-parse version? (now 2.) Or is there any blocker?

Documentation on v2 is very scarce, and I don't get the intent is for this next version as it mentions right away the use of jsdoc2md, and we don't want the markdown transformation https://github.com/jsdoc2md/jsdoc-parse

vvo · 2016-10-19T19:31:08Z

This looks good, only worried about having to change our coding style given limitations of the jsdoc parsing. Would be awesome to fix that.

The not so easy part to grasp is that we are documenting everything with jsDoc comments, there seems to be no documentation inferred from the code itself (Am I right?). So ultimately the way we write code should not even interfere.

Technically we could just do a regexp that extract a jsDoc comment then work from there, if it requires us to just add @name InstantSearch, that's ok I guess. There should not even be a need to entirely parse the JavaScript file.

The parts we had to reformat (.bind(this)) are not used as for the documentation so I hope we can workaround this.

If that's still not doable, the jsdoc-parse author being very responsive we could maybe get something working with him, cc @75lb ;)

75lb · 2016-10-19T20:10:17Z

hi @vvo .. this is a big PR with a lot going on - what do you need me to look at? What is the question?

vvo · 2016-10-19T20:12:39Z

hi @vvo .. this is a big PR with a lot going on - what do you need me to look at? What is the question?

Parsing things like https://github.com/algolia/instantsearch.js/pull/1435/files#diff-872aa81dff71f8151ef16821d4e665f4L22 seems not OK with jsdoc-parse@1. @bobylito will provide more context tomorrow if that's not sufficient. Thanks for jumping in!

75lb · 2016-10-19T20:17:19Z

does the source code parse correctly with jsdoc? If not, your issue is with that tool as jsdoc-parse sits downstream..

vvo · 2016-10-19T20:42:50Z

packages/react-instantsearch/src/widgets/CurrentFilters/index.js

 import connect from './connect';
-import CurrentFilters from './CurrentFilters';
+import CurrentFiltersComponent from './CurrentFilters';


Could we say that any capitalized token is a Component otherwise it's just a function/HOC?

I am not sure suffixing Component everywhere brings more context/value to the code?

For sure we need to understand that some component are connected, some are not but do we actually need to express it everywhere?

Interesting point! Previously we were constructing the widgets in an anonymous way. In every file we had Connected as the variable name. Instead now, we are writing down the name of the widget / component as it is documented. So if you look in the code for Hits you will find matches in the index.js as well, the code is more explicit about the intention and, the @name is now inferred from the code.

vvo · 2016-10-19T20:47:30Z

does the source code parse correctly with jsdoc?

Not sure, still I am curious, from reading the jsdoc-parse I cannot see anywhere we are actually requiring/using the jsdoc module. How does that works? Did you reimplement part of it.

What I was proposing is that since we do not infere anything (it seems) from the code itself but we are just parsing jsDoc comments then there should be no need to actually try to parse the code. Just being able to extract a jsDoc block comment should be sufficient (in our case). And would allow us to keep our coding style.

75lb · 2016-10-19T20:58:35Z

i wrote a simplistic description of how jsdoc2md works here. The raw jsdoc data is passed into jsdoc-parse, which transforms it into something more useful for passing into handlebars.

jsdoc2md works downstream of jsdoc. If you only want to parse jsdoc comments (and not source code, as jsdoc does) then your best option is doctrine.

bobylito · 2016-10-20T07:33:50Z

Technically we could just do a regexp that extract a jsDoc comment then work from there, if it requires us to just add @name InstantSearch, that's ok I guess. There should not even be a need to entirely parse the JavaScript file.

There is some inference from the code: names and types.

The parts we had to reformat (.bind(this)) are not used as for the documentation so I hope we can workaround this.

This is part of stage-2, and basically we have this preset only to support this syntax. Can we work with that and move forward? (the only file being impacted is InstantSearch.js).

We can also workaround that by using babel to process the files prior to using jsdoc-parse. Since this is only syntax sugar, that could work.

If you only want to parse jsdoc comments (and not source code, as jsdoc does) then your best option is doctrine.

doctrine only works on extracted comments which adds yet another step...

vvo · 2016-10-21T05:09:13Z

There is some inference from the code: names and types.

I cannot find where do we infer types from the code, can you detail?

Since this is only syntax sugar, that could work.

If this allows us to keep the same syntax (Static, bindings) then that would be better yes. Mostly so that the PR is easier to read and so that we know we do not have to code in a particular way for the documentation generation to work.

doc(jsdoc): jsDoc for Component & connector documentation

922bfbd

vvo reviewed Oct 19, 2016

View reviewed changes

Merge branch 'v2' into v2-jsdoc-allthethings

3020946

alexkirsz reviewed Oct 19, 2016

View reviewed changes

bobylito force-pushed the v2-jsdoc-allthethings branch from 93a72af to b5df378 Compare October 19, 2016 14:50

doc(jsdoc): Peer review update

1db96ad

bobylito force-pushed the v2-jsdoc-allthethings branch from b5df378 to 1db96ad Compare October 19, 2016 14:53

alexkirsz reviewed Oct 19, 2016

View reviewed changes

vvo suggested changes Oct 19, 2016

View reviewed changes

vvo added the packages/react-instantsearch label Oct 21, 2016

Merge branch 'v2' into v2-jsdoc-allthethings

f1653fc

vvo merged commit 1bc8e63 into v2 Oct 21, 2016

vvo deleted the v2-jsdoc-allthethings branch October 21, 2016 12:09

renovate bot mentioned this pull request Feb 2, 2018

chore(deps): update dependency eslint-plugin-react to v7.6.1 - let renovate recreate this one #2583

Closed

doc(jsdoc): jsDoc for Component & connector documentation #1435

doc(jsdoc): jsDoc for Component & connector documentation #1435

Conversation

bobylito commented Oct 18, 2016

algobot commented Oct 18, 2016

vvo commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobylito commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobylito Oct 20, 2016 • edited

Choose a reason for hiding this comment

vvo commented Oct 19, 2016 • edited

75lb commented Oct 19, 2016

vvo commented Oct 19, 2016

75lb commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvo commented Oct 19, 2016

75lb commented Oct 19, 2016

bobylito commented Oct 20, 2016

vvo commented Oct 21, 2016

bobylito Oct 20, 2016 •

edited

vvo commented Oct 19, 2016 •

edited