fix: Preserve Social Link Order

[lorenzolewis]

What kind of changes does this PR include?

• Changes to Starlight code

Description

• Closes Social links ordering #374
• Preserves the order of social links as defined in the config

The only thing I wasn't a huge fan of in this was losing all the extra "helper text" in the config.

[changeset-bot]

🦋 Changeset detected

Latest commit: d941155

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	Patch

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

[netlify]

✅ Deploy Preview for astro-starlight ready!

Name	Link
🔨 Latest commit	d941155
🔍 Latest deploy log	https://app.netlify.com/sites/astro-starlight/deploys/64c0fc91fbefd60007380841
😎 Deploy Preview	https://deploy-preview-381--astro-starlight.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[delucis]

Would you mind explaining what this does a bit more? By “order”, do you mean the order in which the icons are displayed on the site?

If so, why does this schema change do that? Seems like we might be leaning on a Zod implementation detail, which makes me a bit nervous 😅

[HiDeoo]

Yeah, if I remember correctly, zod parses by looping through the schema keys so the order the user uses in their config won't affect the order of the icons in the header. Maybe accepting an array too could be another approach 🤔

[delucis]

Yeah, anywhere order is important, an array will be more reliable. We’d end up with an API something like this though, so I do wonder if order is important enough to complicate this.

social: [
  { icon: 'twitter', link: 'https://twitter.com/astrodotbuild' },
  { icon: 'github', link: 'https://github.com/withastro/starlight' },
]

Or in theory something like this style which a lot of VitePress config prefers, but I’m not a big fan of the unlabelled arguments personally.

social: [
  ['twitter', 'https://twitter.com/astrodotbuild'],
  ['github', 'https://github.com/withastro/starlight'],
]

[lorenzolewis]

Yeah, if I remember correctly, zod parses by looping through the schema keys so the order the user uses in their config won't affect the order of the icons in the header. Maybe accepting an array too could be another approach 🤔

Yup, this was the issue from what I could find. A little more detail is here: colinhacks/zod#1852.

Only reason I didn't go with an array was because of breaking configs, but could go with whatever y'all would like

[HiDeoo]

Other (prolly way too late) random ideas:

• I guess another approach if we consider it's gonna be a pretty rare use-case to re-order these icons is having a new socialOrder or something like that accepting an array but I am not a fan either.
• Use a CSS grid for the icons, add a class name to each icons matching the social name and let the users re-order them through CSS.

[delucis]

Thanks for the extra context @lorenzolewis! I wonder if the response marked as an answer in that discussion could help us? Had to re-read the code several times because it’s not super clear, but rewritten in a way my brain understands:

function preserveKeyOrder<Schema extends z.ZodObject<z.ZodRawShape>> (
  schema: Schema
) {
  return z.custom((value) => schema.safeParse(value).success) as Schema;
}

const objSchema = z.object({
    one: z.number(),
    two: z.number(),
});
const objSchemaRespectOrder = preserveKeyOrder(objSchema)

console.log(objSchema.parse({ two: 2, one: 1 }));
// { one: 1, two: 2 }

console.log(objSchemaRespectOrder.parse({ two: 2, one: 1 }));
// { two: 2, one: 1 }

So basically uses a custom validator that just passes through the original values if they pass, thereby preserving order. Would let us keep the current shape, just with the extra helper to preserve key order.

[lorenzolewis]

@delucis I did try that a couple of different ways but couldn't get it to work (admittedly I think that was getting a bit too far into zod and TS for me to wrap my head around 😅)

But if someone is able to get it to work then it'd accomplish the same thing without affecting the config API (just maybe a little more complexity in that bit of the codebase that I wish anyone in the future best of luck with 💜 )

[delucis]

Use a CSS grid for the icons, add a class name to each icons matching the social name and let the users re-order them through CSS.

Ah, yeah, this is quite a nice idea either way actually! The icons are already in a flex container, so order works today, there’s just no easy way to target them.

They’re also in a single container with the other items in the header, so I can do fun stuff like this by setting order: 1 on the Discord icon:

If that Zod hack above works I think it would still be nice to have, but maybe it doesn’t and this is a nicer path for those who really want it!

[lorenzolewis]

@delucis I wonder if this could just be a way to compose the whole navbar ordering via some order properties? Like if someone wanted that searcher to be on the trailing edge instead of immediately after the logo? 🤔

Maybe could just add some classes/ids to make them a bit easier to target and then add to the docs on it?

[HiDeoo]

If that Zod hack above works I think it would still be nice to have, but maybe it doesn’t and this is a nicer path for those who really want it!

I agree, and I guess if this works, having this zod "helper" could maybe be useful for some other thing one day.

But if someone is able to get it to work

I can try to take a look at it tomorrow and post my findings here.

[HiDeoo]

So I played with it a little bit and the proposed solution in the discussion works but it has some drawbacks.

First, as we return a boolean in the z.custom() check (we cannot throw as this would break safeParse), we basically lose detailed validation error messages. Right now, if we pass a number instead of a string to a social link, we get an error like this:

**social.discord**: Expected type string, receive number

But if we use the z.custom() check, we get a generic error:

**social**: Invalid input

The usual solution is to use a string as second parameter of z.custom() but this can only return a generic error message which is not very helpful either.

Another approach is to use a function as second parameter of z.custom() but it also has some drawbacks:

• We won't know what the error is about as the checker only returns a boolean and does not have access to the context to add an issue.
• This means we need to re-parse the input a second time to get the errors (this may not be such a big deal as this would only happen for errors).
• This function can only returns parts of a z.IssueData like a message or a path and will not hit the Starlight custom error map. This also means we can only return the first error encountered and not all of them.
• We do not know the actual path of what we are validating, e.g we can know discord is invalid but we don't know it is actually social.discord that is invalid.

Taking all this into account, the best I could quickly come up with is this:

function zOrderedObject<TShape extends z.ZodRawShape>(
  shape: TShape,
  prefix?: string
) {
  const schema = z.object(shape);

  return z.custom(
    (input) => schema.safeParse(input).success,
    (input) => {
      const parsedInput = schema.safeParse(input);
      const firstError = !parsedInput.success
        ? parsedInput.error.errors.at(0)
        : undefined;
      const path = `${prefix ? `${prefix}.` : ""}${firstError?.path.join(".")}`;
      const msg = firstError?.message ?? "Validation error";

      return { message: `**${path}**: ${msg}` };
    }
  ) as z.ZodObject<TShape>;
}

which can be used like this:

{
  social: zOrderedObject(
    {
      /** Link to the main Twitter profile for this site, e.g. `'https://twitter.com/astrodotbuild'`. */
      twitter: z.string().url().optional(),
      /** Link to the main Mastodon profile for this site, e.g. `'https://m.webtoo.ls/@astro'`. */
      mastodon: z.string().url().optional(),
      /** Link to the main GitHub org or repo for this site, e.g. `'https://github.com/withastro/starlight'`. */
      github: z.string().url().optional(),
      /** Link to the Discord server for this site, e.g. `'https://astro.build/chat'`. */
      discord: z.string().url().optional(),
      /** Link to the Codeberg profile or repository for this site, e.g. `'https://codeberg.org/knut/examples'`. */
      codeberg: z.string().url().optional(),
      /** Link to the Youtube channel for this site, e.g. `'https://www.youtube.com/@astrodotbuild'`. */
      youtube: z.string().url().optional(),
      /** Link to the Threads profile for this site, e.g. `'https://www.threads.net/@nmoodev'`. */
      threads: z.string().url().optional(),
      /** Link to the LinkedIn page for this site, e.g. `'https://www.linkedin.com/company/astroinc'`. */
      linkedin: z.string().url().optional(),
      /** Link to the Twitch profile or repository for this site, e.g. `'https://www.twitch.tv/bholmesdev'`. */
      twitch: z.string().url().optional(),
    },
    // This is required to get the proper path in the error message.
    "social"
  ).optional();
}

Not sure how I personally feel about this as it seems a bit too much code to only returns the first error encountered vs all of them which is the case right now.

Any thoughts on this?

[delucis]

Thanks so much for the deep dive @HiDeoo! Appreciate it.

We might be able to show all errors using our existing error map like we do when parsing user config:

starlight/packages/starlight/index.ts

Lines 18 to 25 in af48504

	const parsedConfig = StarlightConfigSchema.safeParse(opts, { errorMap });
	if (!parsedConfig.success) {
	throw new Error(
	'Invalid config passed to starlight integration\n' +
	parsedConfig.error.issues.map((i) => i.message).join('\n')
	);
	}

That said, I still think we might be leaning a bit too much on a detail in Zod, plus worse than the enum solution in this PR, it introduces that code complexity.

I was about to suggest the CSS approach but then I remembered that that messes with tab order, so it’s not the most accessible result.

I think I’d be OK maybe merging this enum approach with the caution that it may break if Zod changes and we won’t necessarily rush to fix it if that does happen.

[lorenzolewis]

There aren't any snapshot tests in the repo yet, right? We could change it so that if this does break in zod we can catch it with a snapshot test.

[delucis]

There aren't any snapshot tests in the repo yet, right?

Unit tests can (and in fact do) include snapshots! A simple test would be to import the user config schema, parse an example options object, and snapshot the resulting social property. I’ve just been using inline snapshots so far, which should be fine for smaller things like this.

Example test:

starlight/packages/starlight/__tests__/basics/navigation.test.ts

Lines 126 to 141 in af48504

	expect(links).toMatchInlineSnapshot(`
	{
	"next": {
	"href": "/guides/authoring-content/",
	"isCurrent": false,
	"label": "Authoring Markdown",
	"type": "link",
	},
	"prev": {
	"href": "/",
	"isCurrent": false,
	"label": "Home Page",
	"type": "link",
	},
	}
	`);

[lorenzolewis]

I added in a snapshot but I think the order gets smugged somewhere through the process: https://github.com/withastro/starlight/actions/runs/5658648982/job/15330419940#step:6:53

Apologies for that fun unverified commit sneaking in, multiple git identities are hard 😮‍💨

[lorenzolewis]

Got git good.

Added that test back in so you can see the fail. It looks like Vitest orders it alphabetically?

https://github.com/withastro/starlight/actions/runs/5659175483/job/15332146634?pr=381

[HiDeoo]

Maybe in this case it would be better to not rely too much on Vitest helpers, maybe Object.keys would be enough, e.g.:

test("preserve social config order", () => {
  const parsedConfig = StarlightConfigSchema.parse({
    title: "Test Title",
    social: {
      github: "https://github.com/withastro/starlight",
      discord: "https://astro.build/chat",
    },
  });

  expect(Object.keys(parsedConfig.social ?? {})).toEqual(["github", "discord"]);
});

Any thoughts?

[lorenzolewis]

Ooofff the more I look at this whole approach the more flakey it seems. I wonder if it'd be better to just switch over to an array sooner rather than later like @delucis suggested 🤔

[delucis]

Fixed the test based on @HiDeoo’s suggestion. Vitest use’s Jest’s pretty-format, which is probably sorting keys on the basis that key order is not important (it generally shouldn’t be) and you don’t want a test to fail because a snapshot object is now { foo: 'bar', baz: null } instead of { baz: null, foo: 'bar' } when those objects are equivalent for pretty much any use case (except ours).

As I said above, merging this on the basis that it’s an easy win — hopefully solves ordering in most cases, while simplifying the social schema a bit with an enum. I don’t think the order of these icons is likely all that important to most people, but if I’m proved wrong, we can revisit and look at making the config an array.

[delucis]

Thanks again @lorenzolewis!