feat(core): Switch to Promises everywhere. Adopt Node v4 ES6 #648

tmcw · 2016-12-29T23:51:38Z

This is a big one: this would switch documentation.js from callbacks to Promises entirely, while also adopting ES6 template strings and arrow functions. I'm following this table for the features to use: only those available in Node v4.

This is a big one, so pinging @arv @jfirebaugh to weigh in. From my perspective right now, this feels really good and I'm going to continue working on it. Promises make a lot of sense for documentation.js's APIs, where the automatic error-passing works pretty well.

There's still more to do on this PR: I expect CI tests to fail.

TODO:

~~Figure out how we're going to handle options for both build & format steps. Right now that messes up some of the tests.~~ Document the approach: running mergeOptions on all Node API endpoints
Figure out how to merge in default options from yargs
Document Promises more accurate re: @dignifiedquire's great suggestion
Fix html output project name regression
~~Realizing that the refactored config system now runs mergeOptions twice if you call build. Should this be nixed?~~: buildSync is no longer an API method.

dignifiedquire · 2016-12-30T08:43:16Z

One note about documenting promise returns, if you use Promise<string> instead of just Promise it's much easier to understand and read in the docs.

arv

Promises and Node4 makes the code easier to reason about. I'm all for it.

I took a quick look at the changes and made some minor comments.

arv · 2016-12-30T21:07:49Z

bin/documentation.js

-  .usage('Usage:\n\n' +
-    '# generate markdown docs for index.js and files it references\n' +
-    '$0 build index.js -f md\n\n' +
+  .usage(`Usage:


You lost one newline here. Is that intentional?

arv · 2016-12-30T21:17:13Z

lib/commands/serve.js

+      .then(files => {
+        watcher.add(files.map(data =>
+          typeof data === 'string' ? data : data.file));
+      }, err => {


General advice is to use then(a).catch(b) instead of then(a, b) since the former would catch errors in a too.

👍 switched all the instances.

arv · 2016-12-30T21:19:03Z

lib/hierarchy.js

@@ -112,8 +110,7 @@ module.exports = function (comments) {
          node.comments.length ? path.concat(node.comments[0]) : []);
      }

-      for (var i = 0; i < node.comments.length; i++) {
-        var comment = node.comments[i];
+      for (var comment of node.comments) {


Your PR description didn't mention for-of? I'm fine with that too even though it is pretty slow in Node4

arv · 2016-12-30T21:20:19Z

lib/infer/membership.js

@@ -97,7 +97,7 @@ function extractIdentifiers(path) {
     * @returns {undefined} has side-effects
     * @private
     */
-    Identifier: function (path) {
+    Identifier: path => {


Or using method syntax? Identifier(path) {

arv · 2016-12-30T21:25:33Z

package.json

    "glob": "^7.0.0",
+    "globals-docs": "^2.3.0",


Maybe add an engines field too?

👍 updated, added engines.

arv · 2016-12-30T21:26:37Z

test/fixture/boolean-literal-type.output.md.json

@@ -13,16 +13,7 @@
          "type": "text",
          "value": "f"
        }
-      ],
-      "data": {


Why did these change?

tmcw · 2016-12-31T21:03:10Z

Well, I'm unplugging and getting a brain-break shortly, but took the next step: using Flow to enforce types. That might look like just 'diving in', but I really think it is a winning move for the long-term success of documentation.js - it turns a lot of implicit types, like 'a comment' and 'a tag' into concrete types that we can enforce.

I'm using the Flow comment syntax, so, at the added cost of 4 extra chars per annotation, it spares us the need to transpile or remove type annotations.

Big changes: * Uses template strings where appropriate * Config and argument parsing is unified and there is no such thing as formatterOptions anymore. All user-passed options go through mergeConfig. More changes: * Remove buildSync command * feat(inference): Partially implement object shorthand support * Refs #649 * Use Flow annotations to enforce types * Keep flow but switch to comment syntax * Clarify types * More flow improvements * Turn server into class * LinkerStack becomes class too * Fix comment description type * Run flow on lint * Many more flow fixes * More intense flow refactoring * Simplify inference steps * Update inference tests, flow errors down to 1 * Continue refining types * Fix more flow issues * Use 'use strict' everywhere * Make 'ast' property configurable * Fix many tests * Fix more tests * Fix more tests * Fix augments * Test Markdown meta support * Improve test coverage

coveralls · 2017-01-27T21:03:31Z

Coverage decreased (-1.2%) to 97.766% when pulling 497ab25 on future-promises into ec03e0b on master.

tmcw · 2017-01-27T21:13:33Z

I'm going to accept the -1.2% coverage increase and merge this, so that we can iterate on this in master.

arv · 2017-01-28T03:14:07Z

This is epic. Thanks.

tmcw force-pushed the future-promises branch 4 times, most recently from e945617 to 62582d5 Compare December 30, 2016 03:25

tmcw force-pushed the future-promises branch 7 times, most recently from 3e7812c to e37ce09 Compare December 30, 2016 17:46

tmcw mentioned this pull request Dec 30, 2016

Question: how could one apply yargs-defined defaults to an object? yargs/yargs#744

Open

arv reviewed Dec 30, 2016

View reviewed changes

tmcw force-pushed the future-promises branch 3 times, most recently from 970d1bb to c437326 Compare December 31, 2016 16:15

tmcw mentioned this pull request Dec 31, 2016

Robust parsing of object method shorthand #649

Closed

tmcw force-pushed the future-promises branch 4 times, most recently from 131628e to e1fc69d Compare January 10, 2017 03:33

tmcw force-pushed the future-promises branch from e1fc69d to d9c4adb Compare January 10, 2017 16:25

Switch back from for of to for for speed

497ab25

tmcw merged commit 631c692 into master Jan 27, 2017

tmcw deleted the future-promises branch January 27, 2017 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Switch to Promises everywhere. Adopt Node v4 ES6 #648

feat(core): Switch to Promises everywhere. Adopt Node v4 ES6 #648

tmcw commented Dec 29, 2016 •

edited

Loading

dignifiedquire commented Dec 30, 2016

arv left a comment

arv Dec 30, 2016

arv Dec 30, 2016

tmcw Dec 31, 2016

arv Dec 30, 2016

arv Dec 30, 2016

arv Dec 30, 2016

tmcw Dec 31, 2016

arv Dec 30, 2016

tmcw commented Dec 31, 2016 •

edited

Loading

coveralls commented Jan 27, 2017

tmcw commented Jan 27, 2017

arv commented Jan 28, 2017

feat(core): Switch to Promises everywhere. Adopt Node v4 ES6 #648

feat(core): Switch to Promises everywhere. Adopt Node v4 ES6 #648

Conversation

tmcw commented Dec 29, 2016 • edited Loading

dignifiedquire commented Dec 30, 2016

arv left a 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

tmcw commented Dec 31, 2016 • edited Loading

coveralls commented Jan 27, 2017

tmcw commented Jan 27, 2017

arv commented Jan 28, 2017

tmcw commented Dec 29, 2016 •

edited

Loading

tmcw commented Dec 31, 2016 •

edited

Loading