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

Investigate failures in twbs/bootstrap #233

Closed
JustinBeckwith opened this issue Jan 10, 2021 · 7 comments · Fixed by #265
Closed

Investigate failures in twbs/bootstrap #233

JustinBeckwith opened this issue Jan 10, 2021 · 7 comments · Fixed by #265
Labels
bug Something isn't working released

Comments

@JustinBeckwith
Copy link
Owner

JustinBeckwith commented Jan 10, 2021

Starting with recentish release, linkinator started failing on a perfectly valid link in the bootstrap docs. The issue was raised here:
twbs/bootstrap#32678 (comment)

This link shows a 404:
https://getbootstrap.com/docs/5.0/

I debugged a little and it turns out to be an issue with the https://github.com/vercel/serve-handler transition, which was done for directory listings. More research needed.

cc @XhmikosR

@XhmikosR
Copy link
Contributor

XhmikosR commented Jan 24, 2021

Not sure if it will help, but after reading the source code a bit I think it'd be better if

  1. this would be moved to a separate function which would return the expandedPaths and use Promise.all maybe?

    linkinator/src/options.ts

    Lines 78 to 107 in cb1d808

    const paths: string[] = [];
    for (const filePath of options.path) {
    // The glob path provided is relative to the serverRoot. For example,
    // if the serverRoot is test/fixtures/nested, and the glob is "*/*.html",
    // The glob needs to be calculated from the serverRoot directory.
    const fullPath = options.serverRoot
    ? path.join(options.serverRoot, filePath)
    : filePath;
    const expandedPaths = await glob(fullPath);
    if (expandedPaths.length === 0) {
    throw new Error(
    `The provided glob "${filePath}" returned 0 results. The current working directory is "${process.cwd()}".`
    );
    }
    // After resolving the globs, the paths need to be returned to their
    // original form, without the serverRoot included in the path.
    for (let p of expandedPaths) {
    p = path.normalize(p);
    if (options.serverRoot) {
    const contractedPath = p
    .split(path.sep)
    .slice(options.serverRoot.split(path.sep).length)
    .join(path.sep);
    paths.push(contractedPath);
    } else {
    paths.push(p);
    }
    }
    }
    options.path = paths;
  2. perhaps use more path API methods in options.ts, like basename, parse, join etc instead of this manual handling of paths?

@XhmikosR
Copy link
Contributor

XhmikosR commented Feb 2, 2021

In fact, it seem any directory with periods will return a 404 wrongfully.

See also vercel/serve-handler#120

Might be worth looking into something else :)

@JustinBeckwith
Copy link
Owner Author

Aye, apologies - been work busy.

@XhmikosR
Copy link
Contributor

XhmikosR commented Feb 4, 2021

No worries, just trying to group all the info together.

@XhmikosR
Copy link
Contributor

XhmikosR commented Feb 5, 2021

I want to also add something obvious that I was forgetting. On Windows, filenames cannot contain periods.

I hit this on another repo with linkinator 2.11.2, and I was like WTH.

But this original issue happens on nix.

@JustinBeckwith
Copy link
Owner Author

I started poking at this last night, and came to a funny conclusion ... I don't think I need an npm module for this at all. Since we control both the server and the client, 90% of web server features aren't useful. I started on a PR to replace this with a plain ol HTTP server. More soon.

@github-actions
Copy link

github-actions bot commented Feb 7, 2021

🎉 This issue has been resolved in version 2.13.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants