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

Custom pages with periods in the slug are reported as invalid #84

Closed
KianNH opened this issue Jan 15, 2025 · 5 comments · Fixed by #85
Closed

Custom pages with periods in the slug are reported as invalid #84

KianNH opened this issue Jan 15, 2025 · 5 comments · Fixed by #85

Comments

@KianNH
Copy link

KianNH commented Jan 15, 2025

Describe the bug

I believe this is effectively the same as #31, but the fix that relies on file.data.astro does not apply to custom pages.

To Reproduce

StackBlitz

  1. Create src/pages/foo.bar.astro.
  2. Create src/content/docs/foo.mdx.
    a. Add title: foo and slug: foo.baz to the frontmatter.
  3. Link to [foo](/foo.bar/) and [foo](/foo.baz/) in a Markdown file.
  4. Run npx astro build, and observe that /foo.bar/index.html and /foo.baz/index.html are generated.
12:05:27 ▶ src/pages/foo.bar.astro
12:05:27   └─ /foo.bar/index.html (+16ms) 
12:05:27 ▶ @astrojs/starlight/routes/static/index.astro
12:05:27   ├─ /foo.baz/index.html (+67ms) 
  1. Only /foo.bar/, the custom page, will be reported as an invalid link.
12:05:35 [ERROR] ✗ Found 1 invalid link in 1 file.
12:05:35 ▶ /
12:05:35   └─ /foo.bar/ - invalid link

Expected behavior

/foo.bar/ is reported as a valid link.

How often does this bug happen?

Every time

System Info

No response

Additional Context

No response

@HiDeoo
Copy link
Owner

HiDeoo commented Jan 15, 2025

I think this may be more a duplicate of #39 where custom pages are not validated.

If you rename src/pages/foo.bar.astro to src/pages/foobar.astro and the link from [foo](/foo.bar/) to [foo](/foobar/), the report would be:

 validating links
14:35:54 [ERROR] ✗ Found 1 invalid link in 1 file.
14:35:54 ▶ /
14:35:54   └─ /foobar/ - invalid link

I mentioned in the linked issue, I cannot really provide the same kind of validation for custom pages, e.g. headings, i18n, fallbacks, etc.

My initial approach to the issue was that if I cannot provide the same level of validation, I should not validate them and these pages should be part of an exclude as I mention in the docs but maybe it would be fine only for these pages to see if it's possible to allow some very basic validation 🤔

@KianNH
Copy link
Author

KianNH commented Jan 15, 2025

That makes sense! I was going to say that we have other custom pages that work but we are actually already excluding them 🤦.

My line of thinking was that the routes appear in sitemaps (which uses the same pages from the astro:build:done hook) but I see what you mean about the level of validation where there could be the false sense of security from "I have anchor checking enabled and it passed" but it'd be impossible to check the headings on those pages.

Now that I know our other custom pages were already excluded and that this isn't the same special characters edge case, I'm happy to follow #39 and close this issue if you want!

I wonder if there's a way to determine "this is a custom page" and add some additional information in the "invalid link" report, such as they are not currently validated and linking to the exclude option.

@HiDeoo
Copy link
Owner

HiDeoo commented Jan 15, 2025

I wonder if there's a way to determine "this is a custom page" and add some additional information in the "invalid link" report, such as they are not currently validated and linking to the exclude option.

That's a great idea, my initial thought is "yes, that may be possible" but I'll experiment and report back.

@HiDeoo
Copy link
Owner

HiDeoo commented Jan 20, 2025

I've opened #85 which should improve the warning for such cases.

When an invalid link is pointing to a custom page, the error type now indicates it:

Image

If the entire report contains at least one such error, the error hint now also shows an additional message with a link to the exclude option documentation:

Image

The PR also includes a dynamic page in a test fixture with a slug including a dot to be sure to avoid errors like #31 with dots.

I'll give myself a few days to think about it more and potential wording improvements, but I think this should be a good improvement nonetheless thanks to your great idea.

@HiDeoo
Copy link
Owner

HiDeoo commented Jan 31, 2025

Sorry for the delay, got sick last week and only got back to this today. Just released version 0.14.2 with the new changes.

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 a pull request may close this issue.

2 participants