Skip to content

Fix regressions from #2451 #2453

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

Merged
merged 5 commits into from
Aug 22, 2018
Merged

Fix regressions from #2451 #2453

merged 5 commits into from
Aug 22, 2018

Conversation

Munter
Copy link
Collaborator

@Munter Munter commented Aug 17, 2018

Marked known false negatives from hyperlink as TODO and added a url rewrite mechanism for old urls while fetching README's

This should put the rebuild branch in the green again

@Munter Munter requested a review from montogeek August 17, 2018 23:49
@@ -1,5 +1,11 @@
const url = require('url');

const beginsWithDocsDomainRegex = /^https?:\/\/webpack\.js\.org/;

const redirects = {
Copy link
Member

Choose a reason for hiding this comment

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

Uhmm, I was thinking of using webpack redirects instead of this string replacement
https://github.com/webpack/webpack.js.org/blob/rebuild/webpack.prod.js#L60

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those would not be evaluated when doing file based link checking I think?

Copy link
Member

Choose a reason for hiding this comment

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

My idea is to put setup the redirect, that plugin creates meta tags with the redirect ()https://github.com/bebraw/redirect-webpack-plugin/blob/master/index.js#L15), so when hyperlink hit that link it would be redirected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That might work. I'm pretty sure hyperlink models meta redirects. But it might show the redirect as an error and indicate that you should update the source url to the redirect target. We can probably mark that check as --todo or --skip though

Copy link
Member

Choose a reason for hiding this comment

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

That would work, also, it isn't possible to ask a mantainer to update https://github.com/webpack-contrib/json-loader?

Archide repositories are git blocked or something?

Copy link
Collaborator Author

@Munter Munter Aug 20, 2018

Choose a reason for hiding this comment

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

All PR's are automatically rejected by GIthub when a repo is archived. I would certainly prefer just to update that README. I obviously already tried and got blocked

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

image

Copy link
Member

Choose a reason for hiding this comment

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

No PRs, just ask the maintainer to push a commit updating that README.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Project README updated. I'll remove the rewriting code. Though some of the refactorings will stay, since I discovered that fragment links had the original source prepended, so some of it actually fixes a bug

@Munter
Copy link
Collaborator Author

Munter commented Aug 22, 2018

I'm merging. Can't have the rebuild branch keep failing

@Munter Munter merged commit 7340aca into rebuild Aug 22, 2018
@Munter Munter deleted the rebuild-fix-readme-badges branch August 22, 2018 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants