Migrate from Grunt to npm scripts #584

IllusionMH · 2018-10-25T05:02:10Z

Fixes #484

This PR reimplements current Grunts tasks with npm scripts with only minor changes to code (mostly ES2015 features and lint fixes) without any change of existing logic.

At this point new npm scripts based build should produce identical state of dist folder in comparison with Grunt based build (with exception of updated REAMDE.md, package.json that now contains additional entries in scripts, maps for files with lint fixes, and removed extra line endings in templates)

New benefits:

No more problems with linting of src/test where extends config property was ignored (as mentioned by reduckted in Enable more TSLint rules from core #491 )
Has starting point for Switch from the custom unit test harness to TSLint's --test #489
Create rule now creates tests in TSLint format
Less dev dependencies
no more global Grunt installation

Things that were "lost" (not re-implemented or there is no clear solution with npm scripts):

Total build time and splits by tasks indication
--force option to continue even if error happens
~~Colorful output for warnings/errors in custom tasks (validations, generation etc.)~~
Watcher that launches tests and lint after changes (will return soon)

Additional changes are added for Debug tasks for VS Code and instructions on how to debug it in general case.

I'd like to discuss questions about not implemented parts and changes in build steps/logic during chat.

IllusionMH · 2018-10-25T11:20:58Z

build-tasks/common.js

@@ -0,0 +1,141 @@
+const path = require('path');


I have no idea for better file name. Any suggestion?

How about splitting into common/files.js for file IO and common/metadata.js for everything else?

build-tasks/create-rule.js

JoshuaKGoldberg

Very exciting!

JoshuaKGoldberg · 2018-10-25T15:28:05Z

README.md

-The `node-debug` command will load Node Inspector in your default browser (works in Chrome and Opera only).
+To debug rules that have Mocha tests use:
+
+    node --inspect-brk ./node_modules/mocha/bin/_mocha --no-timeouts --colors "dist/src/tests/**/*.js"


Nit: would you mind using ``` formatting here? GitHub's renderer thinks the _ before `mocha` is an italic block starter.

```shell node ... ```

I usually do this, but decided to follow already used style. Will change.

JoshuaKGoldberg · 2018-10-25T15:28:53Z

README.md

+
+> NOTE: Run `npm test` before `npm start` to copy all required files to `dist` folder.
+
+> NOTE: If breakpoins are not hit you can try to use `inlineSourceMaps` instead of `sourceMaps` in `tsconfig.json`


Nit: breakpoints

JoshuaKGoldberg · 2018-10-25T15:31:00Z

build-tasks/common.js

@@ -0,0 +1,141 @@
+const path = require('path');


How about splitting into common/files.js for file IO and common/metadata.js for everything else?

build-tasks/common.js

JoshuaKGoldberg · 2018-10-25T15:32:37Z

build-tasks/common.js

+    try {
+        return fs.readFileSync(String(fileName), {encoding: 'utf8'});
+    } catch (e) {
+        console.error(`Unable to read file: ${String(fileName)}. Error code: ${e.code}`);


Nit: is the String(...) necessary? Can we just do Unable to read file: ${fileName}. ...`?

tests/no-relative-imports/allow-siblings/test.js.lint

JoshuaKGoldberg · 2018-10-25T15:34:49Z

package.json

+    "generate:sdl-report": "node build-tasks/generate-sdl-report.js",
+    "generate:rule-metadata": "node build-tasks/generate-rule-metadata.js",
+    "generate:package-json-for-npm": "node build-tasks/generate-package-json-for-npm.js",
+    "create-rule": "node build-tasks/create-rule.js"


Would you mind alphabetizing these rules? I always find it really difficult to scan through lists of tasks unless they're grouped nicely.

It is possible to sort alphabetically by group/single tasks.
But current order inside "group" (e.g. generate:<task>) helps keep test script short enough.
As I've seen - npm-run-all preserves tasks order in case when invoked with as npm-run-all generate:* where order matters. Otherwise full script names should be specified in correct order.

Should I leave order inside group as per execution order (create-rule will be moved up, test & start down etc.) or should I reorder everything and explicitly set order inside test script?

Leaving order inside the groups seems good, thanks!

JoshuaKGoldberg · 2018-10-25T15:38:35Z

build-tasks/create-rule.js

@@ -0,0 +1,78 @@
+const common = require('./common');


const common = require('./common');

I'm not a big fan of import * as exports for collections of miscellaneous functions. It becomes a bit harder to tease them apart & refactor when they're prefixed with a name like that. Let's switch to const { specificOneIWant, otherFunction ] = require('./common');.

IllusionMH · 2018-10-26T23:09:08Z

I've fixed all comments and resolved all that we decided to leave as is (exit code, js.lint files).
Removed compilation step for test-data as we discussed earlier.
Colors are back - validation errors have yellow color, other errors that prevent further execution are red.

Code is ready for final review.

As for watch mode - if it is not critical for this PR, I'll make separate PR a bit later.

JoshuaKGoldberg

LGTM. I'll play with it locally just to be sure...

JoshuaKGoldberg

Played with it locally a bit and couldn't break anything. Thanks so much @IllusionMH! This is a great change and should make development utilities quite a bit smoother. 👏

IllusionMH added 10 commits October 25, 2018 07:37

Fix lint errors

d6ab3d1

Add regular Grunt tasks to npm scripts

6f3ebc2

Add example for testing rules with tslint --test

4805ebd

Migrate custom grunt tasks to node scripts

dd69d89

Add create rule task

4526400

Remove Grunt and related packages

adcdf86

Use ES2015 template literals for build tasks

a005323

Update VS Code debug tasks

d87ecad

Add simler watcher tasks and update README with relevant info

51054ab

Remove unnecessary Grunt install step on CI

af69708

IllusionMH commented Oct 25, 2018

View reviewed changes

build-tasks/create-rule.js Show resolved Hide resolved

JoshuaKGoldberg mentioned this pull request Oct 25, 2018

Use a walk function instead of extending AbstractRule in our new-rule templates #586

Closed

JoshuaKGoldberg suggested changes Oct 25, 2018

View reviewed changes

JoshuaKGoldberg added the PR: Waiting for Author Changes have been requested that the pull request author should address. label Oct 26, 2018

IllusionMH added 5 commits October 26, 2018 21:48

Fix forrmatting and typos in README.md

6671eca

Sorted most scripts alphabeticallt. Renamed ts > tsc

4b84e48

Split common.js. Use destructuring for requires.

0a99c1b

Remove unnecessary test-data compilation

a145cdc

Return colorful outpur for errors. Format template literals

36ac80f

JoshuaKGoldberg added PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! and removed PR: Waiting for Author Changes have been requested that the pull request author should address. labels Oct 27, 2018

JoshuaKGoldberg approved these changes Oct 27, 2018

View reviewed changes

Merge branch 'master' into grunt-npm-migration-484

fa811d1

JoshuaKGoldberg approved these changes Oct 28, 2018

View reviewed changes

JoshuaKGoldberg merged commit bf1c20d into microsoft:master Oct 28, 2018

This was referenced Oct 30, 2018

Add watcher that will run tests and lint #616

Merged

Switch from the custom unit test harness to TSLint's --test #489

Closed

IllusionMH deleted the grunt-npm-migration-484 branch November 4, 2018 23:30

JoshuaKGoldberg added this to the 6.0.0-beta0 milestone Nov 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate from Grunt to npm scripts #584

Migrate from Grunt to npm scripts #584

IllusionMH commented Oct 25, 2018 •

edited

Loading

IllusionMH Oct 25, 2018

JoshuaKGoldberg Oct 25, 2018

JoshuaKGoldberg left a comment

JoshuaKGoldberg Oct 25, 2018

IllusionMH Oct 25, 2018

JoshuaKGoldberg Oct 25, 2018

JoshuaKGoldberg Oct 25, 2018

JoshuaKGoldberg Oct 25, 2018

JoshuaKGoldberg Oct 25, 2018

IllusionMH Oct 25, 2018

JoshuaKGoldberg Oct 25, 2018

JoshuaKGoldberg Oct 25, 2018

IllusionMH commented Oct 26, 2018 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg left a comment


		> NOTE: Run `npm test` before `npm start` to copy all required files to `dist` folder.

		> NOTE: If breakpoins are not hit you can try to use `inlineSourceMaps` instead of `sourceMaps` in `tsconfig.json`

Migrate from Grunt to npm scripts #584

Migrate from Grunt to npm scripts #584

Conversation

IllusionMH commented Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg 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

IllusionMH commented Oct 26, 2018 • edited Loading

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

IllusionMH commented Oct 25, 2018 •

edited

Loading

IllusionMH commented Oct 26, 2018 •

edited

Loading