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

Fix broken redirect #596

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

maddsua
Copy link

@maddsua maddsua commented Mar 22, 2024

This fix covers an edgecase when a link that has search params and resolves to a directory
(think of /checkout?services=setup-cctv which resolves to a file dist/checkout/index.html) is reported as broken.

The issue lies in an oversight in src/server.ts:83, where request url is assumed to not have search params when a slash appended to it to implement a directory redirect.

It's fixed with actually parsing URL and appending slash only to pathname instead of the entire url string. Also, I implemented it as a separate URL object to not make things complicated - one object for the original URL and the other for redirect URL specifically.

@JustinBeckwith
Copy link
Owner

Hey, thanks for the PR! Sorry I didn't get to this sooner. To be honest, it's kind of hard for me to tell from your issue and this PR exactly what was broken before 🤔 Could I trouble you to add a test case that demonstrates what's not working? Verify it's broken before your change, and fixed after? Plenty of comments 😆 Thanks!

@maddsua
Copy link
Author

maddsua commented Jul 1, 2024

Added a test case. It is really similar to the one above it, the only difference is that my fix makes search params work regardless of whether url points to a directory or not.

Also, after pulling new commits I've noticed that typescript started complaining about a type error at:

data = await markedData;

Basically, ts isn't happy about that await

@maddsua
Copy link
Author

maddsua commented Jul 21, 2024

Hello again! Any updates?
Would love to have this fixed as soon as possible. Same goes for any additional changes you may find necessary

@maddsua
Copy link
Author

maddsua commented Jul 29, 2024

For more context, this is how it fails in one of the projects I am working on:

> check-links
> npx linkinator dist --recurse --silent --skip ^https.+

🏊‍♂️ crawling dist
[0] dist\forms\signup?pid=t1_inet_basic
[0] dist\forms\signup?pid=t2_inet_premium
[0] dist\forms\signup?pid=t3_inet_vip
[0] dist\en\forms\signup?pid=t2_inet_premium
[0] dist\en\forms\signup?pid=t3_inet_vip
[0] dist\en\forms\signup?pid=t1_inet_basic
dist
  [0] dist\forms\signup?pid=t1_inet_basic
  [0] dist\forms\signup?pid=t2_inet_premium
  [0] dist\forms\signup?pid=t3_inet_vip
dist\en\
  [0] dist\en\forms\signup?pid=t2_inet_premium
  [0] dist\en\forms\signup?pid=t3_inet_vip
  [0] dist\en\forms\signup?pid=t1_inet_basic
ERROR: Detected 6 broken links. Scanned 144 links in 0.436 seconds.

I am genuinely surprised that this case was not caught before.

@maddsua
Copy link
Author

maddsua commented Aug 14, 2024

Hey @JustinBeckwith , I don't wanna be annoying but any chance of you getting another look at this?

I love open source for giving us all a way of sharing amazing tools an collaborating on them instead of writing everything from scratch or forking everything just to add a small feature or fix.

Would absolutely hate to give up on linkinator because of this small but critical for me issue

@maddsua
Copy link
Author

maddsua commented Oct 21, 2024

@JustinBeckwith maybe you could give an ETA on this one?

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