Page MenuHomePhabricator

Transition Gruntfile.js tasks to NPM scripts (Vector)
Closed, ResolvedPublic2 Estimated Story Points

Description

As in T206069, this task encompasses the work to start transitioning Gruntfile.js tasks to package.json. This task covers all simple transitions such as ESLint and creating tasks for what remains. Delete any relevant dependencies as you go and see Popups for reference.

Developer notes

  • eslint is not using grunt
  • stylelint is not using grunt
  • banana-checker is not using grunt
  • grunt is removed from codebase

Motivation

Via T206069:

  • Relying on Grunt plugin maintainers (e.g. dependant on grunt eslint-plugin upgrades everytime eslint is upgraded. Working now, albeit slower pace, but no guarantee it will in future)
  • Grunt going out of fashion
  • Focus should be on production code and less on tooling
  • Only package.json to npm install, npm start
  • Understanding how Grunt works is an unnecessary and additional tax developers need to make when working with our codebases and it's not clear if it's worth it.
  • Tasks can be run in package.json. Grunt not needed for that purpose.
  • CI faster as no longer needs to download grunt and grunt plugins.

Event Timeline

Jdlrobson triaged this task as Medium priority.
Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.
ovasileva added a subscriber: ovasileva.
MBinder_WMF renamed this task from Transition Vector Gruntfile.js tasks to NPM scripts to [S] Transition Vector Gruntfile.js tasks to NPM scripts.Feb 12 2020, 5:36 PM
MBinder_WMF added a subscriber: MBinder_WMF.

@ovasileva This is a Small as long as @Jdrewniak is doing it :)

We're leaving it in upcoming for you to prioritize.

ovasileva renamed this task from [S] Transition Vector Gruntfile.js tasks to NPM scripts to Transition Vector Gruntfile.js tasks to NPM scripts.Feb 17 2020, 2:03 PM
ovasileva set the point value for this task to 2.

I heavily applaud efforts like this, specifically because of: "Understanding how Grunt works is an unnecessary and additional tax developers need to make when working with our codebases and it's not clear if it's worth it."

Thank you.

npm install pulls 420 modules occupying 40+ MB in each skin and extension I work on...
I'm sold on this.
Sidenote: I wonder if folks are using npm link eslint-config-wikimedia instead.

What script names did you plan?
Ex.:

npm test
npm run lint
npm run lintfix
npm run eslint [-- --fix]
npm run csslint [-- --fix]
npm run mwlint

Next POC patch will show the implementation details.

Change 574636 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] [POC] Replace grunt tasks with npm scripts. Removed unnecessary devDependencies.

https://gerrit.wikimedia.org/r/574636

Change 574893 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/Vector@master] Replace Grunt tasks with npm scripts in package.json

https://gerrit.wikimedia.org/r/574893

@Jdrewniak as local lint rules are strict (which is a good thing) I found that using --fix argument with both linters (as seen in the POC MMV patch) saves me minutes before each commit. Do you have provisions to add this use-case to the scripts?

Demian renamed this task from Transition Vector Gruntfile.js tasks to NPM scripts to Transition Gruntfile.js tasks to NPM scripts (Vector).Feb 27 2020, 9:19 AM

@Jdrewniak If possible please include the following in the patch:

  • Task to auto-fix trivial style errors: "lint:fix" : "npm -s run lint:js -- --fix && npm -s run lint:ss -- --fix && npm -s run lint:i18n",
  • Don't run 'doc' script in 'test: "test" : "npm run lint",
  • Run 'doc' script in 'build': "build" : "npm -s test && npm -s run doc",
  • Test *.json files: "lint:js" : "eslint --cache --max-warnings 0 **/*.{js,json}",
  • Skip the single quotes around the path: "lint:ss" : "stylelint resources/**/*.{less,css}",

For consideration:

  • Auto-update on 'npm install': "stylelint-config-wikimedia": ">=0.9.0" (">=", for all 3 wikimedia modules)

Change 574893 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Replace Grunt tasks with npm scripts in package.json

https://gerrit.wikimedia.org/r/574893

One more thing came up. Running
rm package-lock.json && npm un -g eslint && npm i && lint

results in:

sh: 1: eslint: not found

Thus if package-lock.json is regenerated and committed, CI tests will fail like this.

Change 575809 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] Add NPM script npm run lint:fix to fix trivial lint errors

https://gerrit.wikimedia.org/r/575809

Jdlrobson added a subscriber: Jdlrobson.

Sam will sign this off Wednesday earliest.

Change 576471 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] Port 'storybook' scripts to windows, merge 'jsdoc' into 'doc'

https://gerrit.wikimedia.org/r/576471

Change 576472 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] Add necessary devdependency 'eslint', remove not-explicitly-necessary 'stylelint'

https://gerrit.wikimedia.org/r/576472

Change 575811 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] Add .svg minifier script npm run svgmin

https://gerrit.wikimedia.org/r/575811

Change 576533 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] Separate scripts npm test and npm run build: 'test' won't build documentation

https://gerrit.wikimedia.org/r/576533

Change 576534 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] Add linting *.json to script 'npm run lint:js'

https://gerrit.wikimedia.org/r/576534

Change 576535 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] Rename npm script 'lint:styles' to 'lint:ss' (stylesheet)

https://gerrit.wikimedia.org/r/576535

phuedx removed phuedx as the assignee of this task.Mar 4 2020, 6:00 PM
phuedx added a subscriber: phuedx.

Moving back to Needs Code Review as there are 5 patches that are awaiting review.

Jdlrobson claimed this task.
Jdlrobson removed a project: Patch-For-Review.
Jdlrobson updated the task description. (Show Details)

None of the associated patches look related to the goal of this task which was to remove Gruntfile. The associated patches touch other bits of tooling.

None of the associated patches look related to the goal of this task which was to remove Gruntfile. The associated patches touch other bits of tooling.

Do note that 2 patches fix porting and dependency issues with the original patch to be signed off and all patches are related to porting tasks from grunt.
These are non-blocking and I understand you want to sign off this task, so I'll make a separate ticket to track those patches. Next time please inform of such plan before asking me to split it into 5+ patches, all of which now I have to rebase.

@Demian i have asked you to write tasks and get input from others before writing patches several times now. It saves everyone effort. Please be careful not to test my patience any further.

@Demian i have asked you to write tasks and get input from others before writing patches several times now. It saves everyone effort. Please be careful not to test my patience any further.

And that's what I did and I'm still waiting for responses after weeks. You've been the most (and only) helpful person in those CSS cleanup and documentation patches, so you know ;-) Thank you for your help there, again.

With this Grunt task, fortunately, there was significant engagement and plenty of discussion from 5+ developers - thanks go out to them too -, that might not be easy to notice on gerrit's not-fit-for-purpose UI. I've mentioned a few times that there should be an RfC about introducing a Gitlab instance, or something similar to the dev workflow. Such a misunderstanding is the price of outdated tools.