ATEAM-266: Move docs script out of gulpfile #68
Conversation
jakejrichards
changed the title
scripts/build-docs.js
Outdated
| } | ||
|
|
||
| let relativeLinkExtension = '.md'; | ||
| if (docMdOutputFormat === PC_DEV_CENTER_FORMAT) { |
There was a problem hiding this comment.
KRAMDOWN is legacy and we can probably remove at this point. But, if we want to remove that in a sep PR, we should add it here as it was technically a bridge to an HTML format. i.e., it should have html links and probably should have been better named than KRAMDOWN
scripts/build-docs.js
Outdated
| // If invoked via "npm run docs" or "node scripts/build-docs.js" run immediately | ||
| const calledFromCli = !module.parent; | ||
| if (calledFromCli) { | ||
| buildDocs(); |
There was a problem hiding this comment.
Thoughts on adding a catch handler here, log to the console, and then process.exit(1). This will decouple the impl of buildDocs from it's invoking context.
scripts/build-docs.js
Outdated
| process.exit(1); | ||
| } | ||
|
|
||
| let relativeLinkExtension = '.md'; |
There was a problem hiding this comment.
nit, this should be linkExtension since it's global in this context.
There was a problem hiding this comment.
Yeah, the "relative" here applied to the fact that they were relative links. But then I see that I named it linkExtension in the helper 🙃
scripts/build-docs.js
Outdated
| return Promise.all(docPromises); | ||
| }).catch(reason => { | ||
| let errMsg = 'Documentation generation failed'; | ||
| console.error(errMsg, reason); |
There was a problem hiding this comment.
i think we could reject here with an object. maybe something like {msg, reason}. This lets the caller deal with how to handle ... logging (and which logger to use) and/or process.exit.
This is related to the cli handler comment above.
There was a problem hiding this comment.
I think it makes sense then to not catch the error at all, and just leave it up to the caller to handle the error, right?
There was a problem hiding this comment.
yeah, i think that makes sense here since we'll mostly just catch and log it anyway. if it was an api, maybe we'd come up with a few types of errors (enum) and throw that so callers could handle without being exposed to impl details.
mostly this will just get rid of a log statement here in case it is handled by the caller and the log would be a red herring.
|
Thanks for tackling this! very cool! I diffed the dist from master and this branch and they are identical ... accept for the fixed links and return type of course. |
| @@ -0,0 +1,155 @@ | |||
| /* eslint-env node */ | |||
There was a problem hiding this comment.
Thoughts on adding a shebang and making this script executable? Of course, we could do this later when removing gulp or not do it at all if we want to ask users to call through npm run which makes sense as well.
There was a problem hiding this comment.
I just googled this and didn't even know it was a thing! I figured the only way to do it was an npm script. We can always come back and add it later if it's something we feel we need. I'll note it.
| }); | ||
|
|
||
| gulp.task('clean', function () { | ||
| return del([DEST_DIR]); | ||
| }); | ||
|
|
||
| gulp.task('build', build); | ||
| gulp.task('docs', () => { |
There was a problem hiding this comment.
This name change would change contributing.md. But, we're dumping gulp soon enough and you've got it removed in a later commit, so no need to make that change here.
The first step toward removing gulp and cleaning up the build process is to move the individual scripts out of the gulpfile. This PR just moves the build docs script to its own individual script file that can still be run via "npm run docs".