Add remark dependencies

[MylesBorins]

This is an alternative to #17315 which uses a script tools/updates-remark.sh to update the dependencies

it also processes those folders with dmn and npm dedupe resulting in about 15% less files being tracked

[refack]

I posit that there should be a simpler way. My concern is also with first time contributors who will have a 12% larger repo, and (from my experiance) 30% slower git operations.

I'd be happy to review any edge-cases, and assist in trying to figure out the best way to achieve optimal usability of remark and ESLint without needing to burden the repo. Where can I find examples?

If we want to be extra cautious in the meanwhile I agree that rolling back remark from the make lint target is best.

[gibfahn]

From the previous PR:

Since #16635 was closed we have noticed weird edge cases in multiple code and learns with random artifacts being created in peoples git repositories... this is making it harder for people to submit changes to Node

example of a PR with extra files: #17306
example of a PR to fix the gitignore issue: #17224

@refack blocked on the original PR due to filesize, if they (or anyone else) cannot come up with an alternative PR before the TSC meeting on Wednesday I'm going to escalate this to the TSC to reach consensus on. This is causing very real friction for developers and we should prioritize fixing this ASAP

It would be good to understand the issue more. Did #17224 fix the gitignore issue? Is git not obeying its own gitignore for the package-lock.json? If we're hitting it in this case, we're probably going to hit it again in the future.

I understand these were seen on other people's laptops so debugging is not ideal, maybe in future Code-and-Learns we should focus more on sitting down to debug these edge-cases we come across, in my experience it's a really good way to flush out obscure issues in the contributing flow.

[refack]

#17224 was in regard to tools/doc not tools/remark-cli. AFAIK it's working.
#17306 has been fixed, so I'm not sure which file has leaked into the changeset. I'm assuming it was one of the package-lock.json, so I created #17330 which should fix that.

[refack]

IMHO #17330 fixes the issue in a more direct way.

[addaleax]

I’d say the package-lock.json is the least of the problems that this solves; to me, it’s about:

• Not overriding the js-yaml hack that we use to de-duplicate files. I’ve seen that happen a few times locally for me now … so, ironically the thing which is supposed to prevent file system bloat is now creating it, it seems?
• Getting consistent test results: One of the reasons we’ve been checking in all our dependencies into the repo so far is that it enables us to get test results for a specific commit in the tree without relying on external state. (And who knows, the npm registry might stay around forever but it’s not like that’s an absolute certainty.)
  • For linting this might not be as bad as for the other tests, so if we want to keep the npm install variant working, the logical conclusion would be to remove doc linting from make test
• Ultimately, the doctool itself should probably use remark, so we’ll get rid of marked at some point anyway.

[refack]

P.S. I did not block #16635. I suggested an alternative which was adopted instead.

[refack]

@addaleax I agree that there are several fragile "tricks" in the Makefile. That is why I'm requesting extra scrutiny in drastic changes.

I'm not sure why the js-yaml ignore got into this story?

[addaleax]

@refack Because prior to #17224 that would be a part of the effects of running npm install during the build, and I don’t think npm guarantees not updating dev dependencies during npm install.

[MylesBorins]

I was reading https://larsxschneider.github.io/2016/09/21/large-git-repos which has some insight into when git will slow down for various reasons.

It is suggesting that > 100k files generally acceptable without performance issues.

git ls-files | wc -l claims 23898 files in our repo. Making this PR a 7.5% increase in files in the repo... leaving us at about 1/4 of the number of files that are suggested to start to cause problems.

@refack do you have some benchmarks or numbers you can provide showing that this increase in file count is causing a slow down in day to day operations? Specific operations and performance differences would be helpful. You mention 30% slower operations above, I would be very interested in seeing this in practice.

[refack]

First measurement - Webstorm spin up
no node_modules

With node_modules

y axis is CPU usage %
Green - user code
Red - system code
x axis is ~30s and 60s respectfully

[MylesBorins]

@refack if I'm following the chart above it is in regards to time it takes to start webpack? It seems really odd that the extra 2k files double the start up time...

[joyeecheung]

I think we don't need this one if dmn has been installed globally in L23?

[joyeecheung]

@refack What is the chart measuring? The time it takes to open the node core in Webstorm? Can Webstorm be configured to ignore certain folders unless specifically told not to when you are actually making changes in those folders? (I always do that in vscode with node_modules)

[MylesBorins]

@refack ping

[refack]

Webstorm is indexing only javascript files so those extra 1800 files which are mostly javascript and package.jsons are a big net increase.
Meanwhile I'm researching more alternatives like roll-up, and install-from-zip tools.

If we consider this a pressing issue #17330 offers an improvement to the status quo, and does not preclude adding all the deps later.

[joyeecheung]

@refack But does it have to index those files if the user is not actually making changes related to the tools? I think the node_modules can just be excluded to speed up the indexing, this sounds more like a Webstorm issue?

[MylesBorins]

@refack I'm going to emphasize what joyee is saying... if we are talking bootup / index of a specific editor this can be setup to be ignored in the settings of that editor. The original reason you blocked was due to git performance, do you have any perf related to that

[MylesBorins]

I've rebased off master. As we still do not have consensus on this I am tagging for TSC agenda so that we can discuss on Wednesday

[addaleax]

I am still very much +1 on this.

I was just getting ready to land #17428 and linting wouldn’t pass because – for whatever reason – npm install missed the remark-lint-prohibited-strings dependency while installing.

I think it’s really not great for merging PRs to be broken – even if a workaround is as simple as recreating package-lock.json – because of something like this.

[MylesBorins]

Closing in lieu of a PR incoming from @refack to move the remark linter out of the main testing pipeline

[refack]

Cross-ref #17587