Rewrite Makefile using NPS Scripts. Closes #2352 #3195

TedYav · 2018-01-08T02:18:10Z

Addresses #2352

Updated version of #3194
Commit has correct Git credentials this time and Travis / Appveyor configs are now properly updated.

Description of the Change

Where possible, I tried to exactly duplicate the design of the Makefile using nps. I've tested it by running build and test commands, as well as granular testing of individual commands against a vanilla repo to ensure it's running the same commands.

Important notes:

removed the ==> [<ACTION> :: <SUBACTION>] output from makefile as nps does this automatically
did not recreate make .PHONY as this appeared to be a hack for GNU make. I'm not sure if some kind of file watching capability is desired. Perhaps this can be added back if needed.
removed the dependency check (SRC_FILES variable) from Makefile as I didn't see a big reason to include this in nps build command
code coverage is still enabled by setting COVERAGE environment variable to true. I removed the ability to set this via a command line flag as it created issues when nps spawns child processes
make is now nps build
any scripts which depend on make but were not in package.json will be broken
npm run <xxx> commands are now npm start <xxx> or nps <xxx>
scripts can only be run as nps <xxx> if nps is in user's path. However, npm start <xxx> will work regardless.

Alternate Designs

Possible modifications:

Can use npsUtils.concurrently.nps to run tests in parallel. This has pros and cons.
Can add documentation on how to use nps if needed
I'm tempted to eliminate all the constants from the top for NYC, MOCHA, etc, though I imagine these may be useful in case a developer wants to temporarily direct these scripts to use a different installation

Why should this be in core?

Discussed this briefly with @boneskull on Gitter back in early 2016. Didn't get around to doing it till now (my bad).

Benefits

Makefile gone!

Possible Drawbacks

Any code that depends on make commands will need to be updated.

Applicable issues

This should not be a breaking change, unless there is some code I'm not aware of that depends on make commands (possibly some CI type operations)

TedYav · 2018-01-08T02:21:06Z

Not sure where to configure netlify --> it is failing because npm run buildDocs is now npm start buildDocs.

coveralls · 2018-01-08T02:59:18Z

Coverage increased (+0.05%) to 90.018% when pulling 7c830a6 on TedYav:make-to-npm-scripts into a723b8f on mochajs:master.

coveralls · 2018-01-08T03:39:46Z

Coverage increased (+0.05%) to 90.018% when pulling d6680e7 on TedYav:make-to-npm-scripts into a723b8f on mochajs:master.

TedYav · 2018-01-08T05:16:07Z

Not sure why CI is failing.

Travis failed because a test timed out:

  1) file utils
       "before each" hook for "should accept a glob "path" value":
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

And appveyor appears to be having an issue executing Mocha from the CLI, possibly due to a difference in the way make versus nps runs commands on Windows. Perhaps I need to invert the / delimiter between bin/mocha or add ./ ?:

nps is executing `test.node.bdd` : bin/mocha --ui bdd test/interfaces/bdd.spec
'bin' is not recognized as an internal or external command,
operable program or batch file.
The script called "test.node.bdd" which runs "bin/mocha --ui bdd test/interfaces/bdd.spec" failed with exit code 1 https://github.com/kentcdodds/nps/blob/v5.7.1/other/ERRORS_AND_WARNINGS.md#failed-with-exit-code

Don't have Windows machine near to test.

boneskull · 2018-01-08T05:58:46Z

you could try node bin/mocha

boneskull · 2018-01-08T05:59:15Z

travis test may be flake. you could try to increase the timeout to like 3000ms for that test

coveralls · 2018-01-08T06:44:46Z

Coverage increased (+0.05%) to 90.018% when pulling 72681ef on TedYav:make-to-npm-scripts into a723b8f on mochajs:master.

boneskull · 2018-01-08T06:48:15Z

I'm not sure where the netlify configuration lives

boneskull · 2018-01-09T04:15:20Z

OK, we're waiting on @Munter for information about the netlify failure, but since everything else is passing I can review this. 😝

boneskull

This looks awesome, thanks!

I just have a couple tweaks, but everything looks really solid. great work

boneskull · 2018-01-09T04:16:04Z

Makefile

@@ -1,176 +0,0 @@
-BROWSERIFY := "node_modules/.bin/browserify"


all of this red is the best part of the PR

boneskull · 2018-01-09T04:17:17Z

package-scripts.js

+  return `${prefix} ${MOCHA} ${params}`;
+}
+
+module.exports = {


I would absolutely love it if we added description fields to everything here

Kk — check over them in case anything is off. 👍

boneskull · 2018-01-09T04:18:01Z

package-scripts.js

+    build: `browserify ./browser-entry --plugin ./scripts/dedefine --ignore 'fs' --ignore 'glob' --ignore 'path' --ignore 'supports-color' > mocha.js`,
+    lint: {
+      default: 'nps lint.code lint.markdown',
+      code: 'eslint . bin/*',


even though we're not running this on AppVeyor, it should still work on windows. please use double quotes around globs: 'eslint . "bin/*"'

Gotcha. If anyone has access to a Windows machine too it'd be awesome to do a dry run and be sure these commands work as expected.

boneskull · 2018-01-09T04:19:52Z

package-scripts.js

+    prebuildDocs: 'rm -rf docs/_dist && node scripts/docs-update-toc.js',
+    buildDocs: 'nps prebuildDocs && bundle exec jekyll build --source ./docs --destination ./docs/_site --config ./docs/_config.yml --safe --drafts && nps postbuildDocs',
+    postbuildDocs: 'buildProduction docs/_site/index.html --outroot docs/_dist --canonicalroot https://mochajs.org/ --optimizeimages --svgo --inlinehtmlimage 9400 --inlinehtmlscript 0 --asyncscripts && cp docs/_headers docs/_dist/_headers && node scripts/netlify-headers.js >> docs/_dist/_headers',
+    prewatchDocs: 'node scripts/docs-update-toc.js',


I can't remember if nps supports pre- and post- scripts. does it? when we were running this with npm, prewatchDocs would get run before watchDocs when doing npm run watchDocs

Don't think it does, but the pre and post commands are explicitly inserted into the nps commands for that. I did move prepublishOnly back to package.json as I suspect it wouldn't trigger on npm publish if it was in package-scripts.js

boneskull · 2018-01-09T04:22:09Z

package-scripts.js

+        }
+      },
+      browser: {
+        default: 'nps ' +


would prefer one of a multiline template string or array w/ join

likewise for other multiline strings in this file

boneskull · 2018-01-09T04:23:03Z

package-scripts.js

+const MOCHA = `node ${path.join('bin', 'mocha')}`;
+const TM_BUNDLE = 'JavaScript\\ mocha.tmbundle';
+
+function test (testName, params) {


please add a little comment explaining what this does

The Netlify build configuration currently only resides in their admin panel. This configuration file should hopefully override those settings to allow outside contributors to make refactoring changes Refs #3195

Munter · 2018-01-09T09:07:51Z

@TedYav I've just merged a netlify.toml config file on master. If you rebase you should be able to adjust configuration of the build script on this branch

coveralls · 2018-01-09T10:05:03Z

Coverage increased (+0.05%) to 90.018% when pulling 1ae7483 on TedYav:make-to-npm-scripts into e8b5592 on mochajs:master.

TedYav · 2018-01-09T19:39:59Z

I'm going to rebase and squash my commits. Hopefully Travis failure was a one off and we should be good to go. Let me know if anything else is desired (i.e. updating docs to say npm start <xxx> instead of make <xxx>

coveralls · 2018-01-09T19:55:04Z

Coverage increased (+0.05%) to 90.018% when pulling f8b0029 on TedYav:make-to-npm-scripts into ac1dd70 on mochajs:master.

TedYav · 2018-01-09T21:31:34Z

I noticed another commit dropped TextMate integration. I can eliminate the TM command to even this up. Also can merge master into this branch to fix conflicts if desired.
Edit: went ahead and did it. @boneskull lemme know if you'd like any other changes :)

coveralls · 2018-01-10T19:45:18Z

Coverage increased (+0.05%) to 90.018% when pulling 45ba261 on TedYav:make-to-npm-scripts into c7730a6 on mochajs:master.

boneskull · 2018-01-15T18:20:46Z

Thanks. super awesome!

The Netlify build configuration currently only resides in their admin panel. This configuration file should hopefully override those settings to allow outside contributors to make refactoring changes Refs mochajs#3195

TedYav force-pushed the make-to-npm-scripts branch 3 times, most recently from efde20d to d6680e7 Compare January 8, 2018 03:27

boneskull assigned Munter Jan 8, 2018

boneskull suggested changes Jan 9, 2018

View reviewed changes

boneskull added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Jan 9, 2018

Munter mentioned this pull request Jan 9, 2018

Added Netlify config file #3197

Merged

TedYav force-pushed the make-to-npm-scripts branch from c91569c to 1ae7483 Compare January 9, 2018 09:48

boneskull unassigned Munter Jan 9, 2018

boneskull added the type: chore generally involving deps, tooling, configuration, etc. label Jan 9, 2018

TedYav force-pushed the make-to-npm-scripts branch from 1ae7483 to f8b0029 Compare January 9, 2018 19:41

Rewrite Makefile using NPS Scripts. Closes mochajs#2352

45ba261

TedYav force-pushed the make-to-npm-scripts branch from f8b0029 to 45ba261 Compare January 10, 2018 19:38

boneskull approved these changes Jan 15, 2018

View reviewed changes

boneskull merged commit 0a3e32b into mochajs:master Jan 15, 2018

TedYav deleted the make-to-npm-scripts branch January 15, 2018 23:17

boneskull added this to the v5.0.0 milestone Jan 17, 2018

boneskull added area: repository tooling concerning ease of contribution and removed type: chore generally involving deps, tooling, configuration, etc. labels Jan 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite Makefile using NPS Scripts. Closes #2352 #3195

Rewrite Makefile using NPS Scripts. Closes #2352 #3195

TedYav commented Jan 8, 2018

TedYav commented Jan 8, 2018

coveralls commented Jan 8, 2018

coveralls commented Jan 8, 2018

TedYav commented Jan 8, 2018

boneskull commented Jan 8, 2018

boneskull commented Jan 8, 2018

coveralls commented Jan 8, 2018

boneskull commented Jan 8, 2018

boneskull commented Jan 9, 2018

boneskull left a comment

boneskull Jan 9, 2018

boneskull Jan 9, 2018

TedYav Jan 9, 2018

boneskull Jan 9, 2018

TedYav Jan 9, 2018

boneskull Jan 9, 2018

TedYav Jan 9, 2018

boneskull Jan 9, 2018

boneskull Jan 9, 2018

boneskull Jan 9, 2018

Munter commented Jan 9, 2018

coveralls commented Jan 9, 2018

TedYav commented Jan 9, 2018

coveralls commented Jan 9, 2018

TedYav commented Jan 9, 2018 •

edited

Loading

coveralls commented Jan 10, 2018

boneskull commented Jan 15, 2018

		@@ -1,176 +0,0 @@
		BROWSERIFY := "node_modules/.bin/browserify"

Rewrite Makefile using NPS Scripts. Closes #2352 #3195

Rewrite Makefile using NPS Scripts. Closes #2352 #3195

Conversation

TedYav commented Jan 8, 2018

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

Applicable issues

TedYav commented Jan 8, 2018

coveralls commented Jan 8, 2018

coveralls commented Jan 8, 2018

TedYav commented Jan 8, 2018

boneskull commented Jan 8, 2018

boneskull commented Jan 8, 2018

coveralls commented Jan 8, 2018

boneskull commented Jan 8, 2018

boneskull commented Jan 9, 2018

boneskull 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Munter commented Jan 9, 2018

coveralls commented Jan 9, 2018

TedYav commented Jan 9, 2018

coveralls commented Jan 9, 2018

TedYav commented Jan 9, 2018 • edited Loading

coveralls commented Jan 10, 2018

boneskull commented Jan 15, 2018

TedYav commented Jan 9, 2018 •

edited

Loading