@nodejs/build

This seems a bit complex, couldn't we just print a warning if the md linter fails that says that you might need to run lint-md-build to update your remark dependencies?

(I haven't looked at this in great detail, so it's quite possible that wouldn't work, let me know if so!)

@joyeecheung @Trott @gibfahn Any comments about this? Thanks!!

@joyeecheung @Trott @gibfahn Any comments about this? Thanks!!

I usually defer to others on Makefile stuff (which is why I pinged @nodejs/build). Two other people (besides @joyeecheung already pinged above) who have worked on Makefile a bit in the past few months: @bnoordhuis @refack

Not sure what people will think of pulling package.json dependencies out with a pile of sed commands and/or what alternatives might be suggested. (I imagine the Makefile can't assume npm is installed, for example.)

If no one reviews this, I'll take a closer look and provide feedback or approve, but I think there are more qualified people than me to comment on Makefile changes.

I don't think it's necessary to print every missing dependencies since the user is only going to run make lint-md-build to fix it instead of running npm install commands individually, but I am not against it either.

@joyeecheung @Trott @gibfahn Any comments about this? Thanks!!

Do you mean in addition to this one?

This seems a bit complex, couldn't we just print a warning if the md linter fails that says that you might need to run lint-md-build to update your remark dependencies?

@gibfahn Sorry the lateness,

This seems a bit complex, couldn't we just print a warning if the md linter fails that says that you might need to run lint-md-build to update your remark dependencies?

If there is an error in the file .remarkrc we are not going to log the error properly and it will say "run lint-md-build" but it would not solve the issue...

ping @gibfahn

Hello @estrada9166, IMHO as well, this a fairly complex solution. Makefile is already too complex to be easily managed. Also there is already an, admittedly less subtle, solution in the code with make lint-md-build or even make lint-md-clean && make lint-md-build.

If I think about the problem space, a possible simpler solution would be to differentiate tool failure (such as a missing dep) from lint errors, and providing better feedback. If you manage to implement that, I would rather support that.

Bottom line, when evaluating cost/benefit of the current solution, I'm don't feel it's worth the added complexity for fixing an edge case with a side tool 🤷‍♂️

If there is an error in the file .remarkrc we are not going to log the error properly and it will say "run lint-md-build" but it would not solve the issue...

Okay, to check I'm understanding what you're saying,

If one of the dependencies fails to install, (e.g. you do make lint-md-build; rm -r tools/remark-preset-lint-node/node_modules/plur), then when you run make lint-md you'll get lots of errors. Running make lint-md-build won't help because it won't remove the node_modules and reinstall. I can see that that could be a problem.

I also take your point that if we do what I suggested and always say "try running lint-md-clean && lint-md-build" then we'd be giving that suggestion even if there was a legitimate docs error.

I understand that that's an issue, but I basically agree with @refack that this is a lot of complexity in a Makefile that is already very complex.

In the discussion following #18981 (comment), decision was that making lint-md-build a dependency of lint-md is a bad idea, because when lint-md-build hasn't been run before lint-md should skip and return nothing rather than trying to npm install (and failing if you're offline).

However, in the case we're talking about here, lint-md-build has already been run before, so the linting won't be skipped.

I don't think Makefiles have a concept of an optional dependency, but basically we could say that lint-md should depend on lint-md-build only in the ifneq ("","$(wildcard tools/remark-cli/node_modules/)") case.

So maybe the answer would be to run lint-md-build if that if condition passes, it takes about 0.4s on my machine, so shouldn't add too much overhead (and it works offline).

Another option is to refactor out all of the doc linting (maybe even the doc building) into a seperate file. IMHO that would be a valuable contribution, but that might also be contentious, unless the UX will be kept simple as it is now.

My biggest concern is the case that happened, and it is when a new remark-lint dependency is installed; if a new dependency is installed and you pull the code, it would not pull the new package on node_modules so...

I don't think Makefiles have a concept of an optional dependency, but basically we could say that lint-md should depend on lint-md-build only in the ifneq ("","$(wildcard tools/remark-cli/node_modules/)") case.

it will pass and it will throw the well-known error.
The main reason I implemented this approach was that if there's a new remark-lint dependency and you already have the node_modules without the new one, you will not know the reason of the error at the beginning, making you debug it, like in this case

My biggest concern is the case that happened, and it is when a new remark-lint dependency is installed; if a new dependency is installed and you pull the code, it would not pull de new package on node_modules so...

I don't think that's true, the remark-lint package.json is checked into git, and @joyeecheung's PR means that if the package.json changes then lint-md-build will rerun npm install in that directory, which should install the new packages (see #18981 (comment)).

@gibfahn I'm not sure that the issue is solved at all, check this issue

@gibfahn I'm not sure that the issue is solved at all, check this issue

What I'm saying is that make lint-md-build will resolve that issue, so this PR can just have make lint-md always call make lint-md-build, which will no-op normally, but will update the node_modules if there has been a change in the package.json.

Just to check if I understand, the suggestion is to call make lint-md-build every time that make lint-md is call?

Just to check if I understand, the suggestion is to call make lint-md-build every time that make lint-md is call?

My suggestion (emphasis on the my, others may disagree) is that we do it if the ifneq condition is true.

The only thing with ifneq is that if we check node_modules; it still pass if the node_modules folder exists but is missing a dependency.

And running make lint-md-build every time, is avoided for this

The only thing with ifneq is that if we check node_modules; it still pass if the node_modules folder exists but is missing a dependency.

And running make lint-md-build every time, is avoided for this

Okay, let me explain myself clearly, and you can tell me if it makes sense.

My proposal is roughly:

ifneq ("","$(wildcard tools/remark-cli/node_modules/)")
+  make lint-md-build # Probably won't look exactly like this
else
# Still skips as it does on master
endif

There are two conditions:

1. Have you run lint-md-build at least once (i.e. does the node_modules folder exist), this is true if the ifneq passes.
2. Has the package.json has changed since you last ran lint-md-build (whether your node_modules is out of date).

If 1. is false, then we should just skip, and not try to run lint-md-build for you. This is handled because the else will fire, so we won't run it.

If 1. is true then we are always okay to run lint-md-build, because

• either: 2. is false, your node_modules are up to date, and the lint-md-build will do nothing.
• or: 2. is true, your node_modules are not up to date, and you need to run lint-md-build, otherwise your lint will fail.

If 1. is true and 2. is true, then running lint-md-build should fix your node_modules, as @joyeecheung explained in #18981 (comment).

I can make the change to run make lint-md-build every time that make lint-md is call; the only thing is this comment:

^ That's true. We tried to avoid internet access.
Context: #16628

This will likely be superseded by #20109

What's the current status on this one?

I think this can be closed as the issues that it addresses have been closed and an alternate solution appears to have landed. Thanks for the pull request, @estrada9166, and if I'm wrong about this and this should remain open, leave a comment (or re-open it if GitHub presents a re-open button).