Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add linkinator. #2565

Merged
merged 1 commit into from
Sep 30, 2019
Merged

Add linkinator. #2565

merged 1 commit into from
Sep 30, 2019

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Sep 9, 2019

Currently only scans the English files only.

I think we'll need to move this a script and use linkinator's API to loop through all languages.

Note that this solution works on Windows too hence why I don't use & in the script. Also, note that I had to add a delay since the npm run serve scripts was starting after linkininator.

Upstream discussion: JustinBeckwith/linkinator#82

Any help is welcome.

@richardlau
Copy link
Member

Just for reference, I abandoned a PR in core that ran linkinator on the API docs: nodejs/node#27267

There were some false positives (mainly links to Google's V8 docs, JustinBeckwith/linkinator#24).

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Sep 9, 2019

Yeah, this is for internal links, mostly. Hence why I suggest to clean up absolute vs relative links so that we manage to at least scan successfully the internal links properly.

@XhmikosR
Copy link
Contributor Author

OK, this only fails due to #2564.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Sep 28, 2019

This should pass now. It should be better than nothing, and assuming we extend it later to scan all locales, it should be a good step forward.

Still doesn't scan 3rd-party links, nor nodejs.org links but we can tweak it further.

@XhmikosR XhmikosR marked this pull request as ready for review September 28, 2019 15:09
@XhmikosR
Copy link
Contributor Author

Forgot to emphasize, it's definitely not a bulletproof solution. I mean, for starters, we shouldn't need to run our server, but it doesn't work with the current dist folder structure.

So, we definitely need to do this via the API later.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Sep 28, 2019

For example, I don't get why the broken links I fixed on #2604 weren't reported.

EDIT: could be because they point outside of /en maybe

Currently only scans the English files only.
@alexandrtovmach
Copy link
Contributor

@XhmikosR Why we can't use this for all languages?

@XhmikosR
Copy link
Contributor Author

It doesn't work with our folder structure, so doing linkinator build/ just doesn't work.

We need to switch to using the API later for each folder, see JustinBeckwith/linkinator#82 and a very first patch master...XhmikosR:master-xmr-linkinator-2

Copy link
Contributor

@alexandrtovmach alexandrtovmach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try this tool. Sounds optimistic

@Trott Trott merged commit 7292e53 into nodejs:master Sep 30, 2019
@XhmikosR XhmikosR deleted the master-xmr-linkinator branch September 30, 2019 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants