nit: no parenthesis: forEach(file => { ...

Fixed.

Could we do this in JS?

• create a new file in tasks folder.
• Use https://github.com/tcort/markdown-link-check/blob/master/markdown-link-check as reference to read file, replace localhost and then call markdownLinkCheck() JS method instead of the binary.
• call the newly created task from here similar to other tasks like compile.

my experience with BASH has always involved some incompatibility between platforms, would be nice to avoid it.

the yarn.lock file needs updating. if you use yarn add markdown-link-check it would add it to both files.

Done.

Fixed.

Done.

We already import all the functions defined in tasks/*, we should be able to make this just be:

checkLinks(docFiles);

Unless there is something I am missing?

Good point. Done. I was following the example set in pr-check.js for the other gulp tests.

to make my comment above work we need to:

exports.checkLinks(files) {///}

then we can get rid of the argv logic here and remove execOrDie

The reason I inserted the argv logic is to enable the use case where someone makes a local change and wants to do "gulp check-links --files FOO.md" before committing / pushing. Removing the argv logic will break this use case.

I have added the export and removed execOrDie. Done.

got it. makes sense. let me know when ready for final look.

This is now ready for a final look. See https://travis-ci.org/ampproject/amphtml/builds/228915159 for a sample.

(Note that gulp has some issues where it reports the wrong time, but I'll look into that in a separate PR. BTW, this will now work locally from the command line too, like so: "gulp check-links --files dir_a/FOO.md,dir_b/BAR.md")

Let's call markdownLinkCheck directly. This way we don't need to rewrite the filtered file back to the filesystem either.

Something like (untested) :

function runLinkChecker(markdownFile) {
  var filteredMarkdownContent = filterLocalhostLinks(markdownFile), 

   var opts = {};
   opts.baseUrl = 'file://' + path.dirname(path.resolve(filepath));

    markdownLinkCheck(filteredMarkdownContent, opts, function (err, results) {
        results.forEach(function (result) {
            if(result.status === 'dead') {
                error = true;
                console.log('[%s] %s', 'X', result.link);
            }
        });
        if(error) {
            console.error('\nERROR: dead links found!');
            process.exit(1);
        }
    });
}

Refactored and rewritten based on one of my previous commits.

see my comment below, if we go with that approach, we can just return filteredontents; here

Refactored. Done.

see my comment below, if we go with that approach this becomes:

runLinkChecker(markdownFile);

change to docFiles = files.filter(isDocFile);

Done.

markdownFiles.forEach(runLinkChecker);

Done.

lets make the branching simpler by assigning early and doing an if (!files) check.

Done.

nit, remove space between "function" and "("

here and below

Done.

we don't have q as a dependency in package.json use bluebird instead: var BBPromise = require('bluebird');

Done.

  var promise = new BBPromise(r => {
    resolve = r;
  });

....

return promise;

Done, without arrow function. Gulp needs to be ES5 compatible or some machines will not be able to run "gulp test".

var resolve

Done.

resolve();

Done.

This could be done with Promise.all and a map, avoiding the global resolve.

I tried this exact approach yesterday and it did not work, most likely because markdownLinkCheck (a third party library) calls linkCheck, yet another (asynchronous) third party library, which invokes the callback that's passed in here.

Can you point to the commit? I'm 100% confident this will work correctly, because it'd be unbelievably taboo for an npm package to break nodeback conventions.

I didn't commit it, since it failed when I gulp tested it locally. I can put it together once again.

See latest commit.

It's weird that a filter function does that actual reading.

Good point. This isn't necessary any more. Moved two lines into the caller. Done.

Converting markdownLinkCheck into a promise returning function makes it easier to deal with. BB provides .promisify to do so:

// top of file
var markdownLinkCheck = BBPromise .promisify(require('markdown-link-check'));

// here, now returns a promise
return markdownLinkCheck(filteredMarkdown, opts).then(results => {
  // do stuff
});

See comment above.

This'll be noisy.

Intentional, since this will only be run in the relatively rare instance when a documentation change is made. The typical PR won't have this task run against it.

results will always be undefined here, as error will cause the promise to reject (and is thus not present in the fulfilled path). Rename error to results, and remove the second param.

Good catch. That was the part I missed when I switched to using bluebird promises. Fixed.

This should be a #map, now.

Fixed.

This is a #map:

var checks = markdownFiles.map(function(file) {
  return runLinkChecker(file);
});
return BBPromise.all(checks);

Fixed.

So this will fail the build after the first failed link in a single markdown file, but there may be others in other files. It'd be more helpful to check all markdown files, then fail at the end. We can do that by not going through all the errors now, but after the Promise.all call (which will receive an array of results).

I exited immediately in keeping with the pattern in pr-check, which bails at the first error. You make a good point about being able to fix all links in all files in one shot. Done.

Looking good at https://travis-ci.org/ampproject/amphtml/builds/229325351. Tested failure case locally. This also works from the command line as "gulp check-links --files dir_a/FOO.md,dir_b/BAR.md".

@jridgewell If there are no more comments, can you merge this?