Refactor package.json
Closed, DeclinedPublic
Actions

Assigned To

None

Authored By

	• nnikkhoui
	Oct 19 2020, 5:30 PM

Description

According to a conversation on T257582, one of the Jenkins pipeline jobs that is always run on skins and extensions mwgate-node10-docker expects npm test to be responsible for linting and nothing else. It was suggested by @hashar to create a secondary script that would run the linters and the build. For example:

{
  "scripts": {
    "test": "npm -s run lint",
    "build": "webpack -p && bin/diff-bundle && tsc",
    "validate": "npm run test && npm run build && npm run doc",
    "doc" : "npm -s run doc"
  }
}

This task is just for shuffling around the npm scripts to adhere to CI expectations, and I believe this would be on the feat/search branch as that has a different package.json setup than master.

Details

	Subject	Repo	Branch	Lines +/-
	Refactor package.json: npm test should only lint	mediawiki/skins/Vector	master	+6 -5
	Refactor package.json: npm test should only lint	mediawiki/skins/Vector	feat/search	+15 -8

Customize query in gerrit

Related Objects

Mentioned Here: T138401: Replace jsduck with JSDoc3 across all Wikimedia code bases
T246326: Process template: How to transition Gruntfile.js tasks to NPM scripts

Event Timeline

• nnikkhoui created this task.Oct 19 2020, 5:30 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 19 2020, 5:30 PM

We've done npm run lint for Vector: https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/574893/3/package.json#5
Somewhat related: T246326: Process template: How to transition Gruntfile.js tasks to NPM scripts

I think the idea would be to shuffle it around so that npm test runs what is being run in npm run lint in the link you provided!

Sorry, I misunderstood the example as the intended target.
IMO ideally npm test would run the linter, unit tests and integration tests (latter in core only). npm test not running actual (unit) tests is counterintuitive.
On a side note in the Vector example running npm run doc in test is also questionable: it multiplies the runtime of test often unnecessarily, IMO that should be in build.
Is there a reason to not run npm run lint in mwgate-node10-docker instead? That would be more semantic, but I don't see that discussed.

For npm run doc, the example is incorrect it looks like docs suggest using a dedicated npm run doc script.

I agree that npm test semantically sounds like it would run all tests, and that putting npm lint in mwgate-node10-docker makes a lot of sense but I'm unsure how many projects that would affect, if they weren't set up with a dedicated lint script that might be better answered by someone on RelEng.

• nnikkhoui updated the task description. (Show Details)Oct 20 2020, 5:43 PM

In the interest of moving this project forward which is already overly ambitious, aligning to whatever conventions RelEng has for all CI projects makes sense to me for the short term. For everyone except CI automation experts, I agree that going against standard expectations for NPM scripts is confusing though so please consider documenting discrepancies you identify. Obviously, changing all the CI infrastructure as part of this task though is out of scope.

Change 635392 had a related patch set uploaded (by Nikki Nikkhoui; owner: Nikki Nikkhoui):
[mediawiki/skins/Vector@feat/search] Reafactor npm scripts

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

gerritbot added a project: Patch-For-Review.Oct 20 2020, 9:47 PM

"doc" : "npm -s run doc" is infinite recursion, did you mean "doc" : "jsduck" or "doc": "jsdoc -c jsdoc.json" (depending on the status of T138401)?

In T265927#6565455, @nnikkhoui wrote:

putting npm lint in mwgate-node10-docker makes a lot of sense but I'm unsure how many projects that would affect, if they weren't set up with a dedicated lint script

All skins and extensions. Hundreds, not counting these:
https://codesearch.wmcloud.org/search/?q=%22lint%22%3A&i=nope&files=%5Epackage.json&repos= (<22)
https://codesearch.wmcloud.org/search/?q=npm%20(-s%20)%3Frun%20lint&i=nope&files=%5Epackage.json&repos= (<16)

It's possible to do it without a massive migration though by falling back on npm run test if npm run lint is missing. This requires a little extra effort to read package.json as npm does not have this functionality built-in, nor to query the existence of a script other than grepping the output of npm run.

This is however not the part of this ticket, but rather the concern of the long-term plans. I was inquiring about that.

Jdlrobson moved this task from Needs Analysis to Upcoming on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.Oct 21 2020, 4:06 PM

"doc" : "npm -s run doc" is infinite recursion, did you mean "doc" : "jsduck" or "doc": "jsdoc -c jsdoc.json" (depending on the status of T138401)?

Yep...haha you're right that example is incorrect, the currently uploaded patch is correct. I hadn't noticed a doc script already exists!

And yes I agree with you and @Niedzielski it is out of scope of this ticket, I'm not sure when that standard was put into place.
Maybe it would help by first documenting it more explicitly so future extensions and skins would know to do the same when they start adopting PipelineLib.

Change 643338 had a related patch set uploaded (by Nikki Nikkhoui; owner: Nikki Nikkhoui):
[mediawiki/skins/Vector@master] Refactor package.json: npm test should only lint

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

Change 635392 abandoned by Nikki Nikkhoui:
[mediawiki/skins/Vector@feat/search] Refactor package.json: npm test should only lint

Reason:

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

Jdlrobson moved this task from Upcoming to Code Review on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.Dec 4 2020, 6:21 PM

Jdlrobson assigned this task to • nnikkhoui.Dec 4 2020, 7:36 PM

Jdlrobson moved this task from Code Review to Doing on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.

ovasileva moved this task from Doing to Needs More Work on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.Jan 5 2021, 6:18 PM

@nnikkhoui: I took a quick look at https://gerrit.wikimedia.org/r/643338 and it looks like it needs a little more work. Is there anything that we (Readers Web) can help with or is this task blocked on RelEng?

@phuedx It's not necessarily blocked on releng, but I had some open questions on the original patch that would help me confidently fix this patch and its parent. (I think it got lost when i abandoned it when we removed the feat/search branch and so thats my bad!

@hashar Would you be able to clarify/respond to my follow up on your comments from here ? I know its been awhile and context might have been lost, but I wanted to be sure that the patch to modify package.json with its parent patch that makes changes to the blubber config, were correct changes with the comments you had given.