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

Error for sidebar entries that match multiple entry schemas #2031

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

Conversation

delucis
Copy link
Member

@delucis delucis commented Jun 18, 2024

Description

  • Closes Conflicting sidebar options are silently ignored #2019
  • Previously, a sidebar entry was parse loosely, e.g. { label: 'Foo', link: '/bar/', items: [] } matched the SidebarLinkItemSchema (it had a label and a link) and was considered valid with items being ignored.
  • This change makes those schemas strict so they may only contain known keys, making the above example an error.

Considerations

  • The Zod error map message is not super helpful (you can see it in the new tests added), but does at least show the index of the item causing the issue.
  • This might make extending the sidebar schema more difficult if people are doing that? IIRC you can’t do z.object({}).strict().extend({ ... }), because the .strict() is already baked in at that point. In practice, extending the sidebar schema may already be too difficult for anyone to be doing it? Still, would want some feedback on whether this is a change we definitely want, or if we’re happy with the loose parsing and to mark Conflicting sidebar options are silently ignored #2019 as working as expected.

Copy link

changeset-bot bot commented Jun 18, 2024

🦋 Changeset detected

Latest commit: 15f7303

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/starlight Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Jun 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Jul 1, 2024 6:42pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
starlight-i18n ⬜️ Ignored (Inspect) Jul 1, 2024 6:42pm

@github-actions github-actions bot added the 🌟 core Changes to Starlight’s main package label Jun 18, 2024
@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Jun 18, 2024

size-limit report 📦

Path Size
/index.html 5.81 KB (-0.02% 🔽)
/_astro/*.js 21.95 KB (-0.02% 🔽)
/_astro/*.css 13.66 KB (0%)

@HiDeoo
Copy link
Member

HiDeoo commented Jun 19, 2024

  • This might make extending the sidebar schema more difficult if people are doing that? IIRC you can’t do z.object({}).strict().extend({ ... }), because the .strict() is already baked in at that point.

Aren't you thinking of transform(), refine() and superRefine() which returns a type with ZodEffects which would require you to use and()? (which would also no longer be true in Zod 4)

Altho, I have to say I've never seen anyone even trying to extend this schema personally so I think the slightly better DX (knowing the index of the sidebar item with an issue is already a good addition imo) makes sense.

@delucis
Copy link
Member Author

delucis commented Jun 19, 2024

Aren't you thinking of transform(), refine() and superRefine() which returns a type with ZodEffects which would require you to use and()? (which would also no longer be true in Zod 4)

IIRC when I last tried, .strict() only applies to the initial object, which means it breaks when extended, for example:

z
	.object({ a: z.string() })
	.strict()
	.extend({ b: z.string() })
	.parse({ a: 'A', b: 'B' })
//  ^ Fails because `b` is not permitted in the initial strict object

Anyway we don’t really support extending the user config now I think about, only frontmatter schema. So should indeed be fine.

@delucis delucis added the 🌟 minor Change that triggers a minor release label Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 core Changes to Starlight’s main package 🌟 minor Change that triggers a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conflicting sidebar options are silently ignored
3 participants