Skip to content

Replace js-yaml with yaml - #4218

Open
delucis wants to merge 10 commits into
mainfrom
chris/yaml
Open

delucis wants to merge 10 commits into
mainfrom
chris/yaml

Conversation

@delucis

@delucis delucis commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Description

  • This PR replaces the js-yaml dependency in @astrojs/starlight with yaml as recommended by e18e. (yaml is significantly smaller and we have been avoiding updating js-yaml to latest in both Starlight and Astro due to recent increases in install size.)
  • It should not be merged before Switch from js-yaml to yaml astro#18099 which does the same thing for astro as a whole to avoid users having both js-yaml and yaml in the dependency tree.
  • There are a couple of snapshot updates here due to a change in how yaml formats stringified data by default. We could avoid it by setting singleQuote in the stringify options, but it seemed like there was no need to enforce the old style necessarily so we may as well use the default of yaml.

To-do

@changeset-bot

changeset-bot Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 14b5364

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

netlify Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for astro-starlight ready!

Name Link
🔨 Latest commit 14b5364
🔍 Latest deploy log https://app.netlify.com/projects/astro-starlight/deploys/6ac663e2205f9c000999d0e7
😎 Deploy Preview https://deploy-preview-4218--astro-starlight.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 99 (🔴 down 1 from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 100 (no change from production)
PWA: -
View the detailed breakdown and full score reports
🤖 Make changes Run an agent on this branch

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

@github-actions github-actions Bot added the 🌟 core Changes to Starlight’s main package label Sep 24, 2026
@astrobot-houston

astrobot-houston commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

size-limit report 📦

Path Size
/index.html 6.1 KB (-0.02% 🔽)
/guides/example/index.html 5.57 KB (0%)
/_astro/*.js 25.49 KB (+0.23% 🔺)
/_astro/*.css 14.84 KB (+0.77% 🔺)

Comment on lines -38 to +36
filePath.ext === '.json'
? JSON.parse(content)
: yaml.load(content, { filename: fileURLToPath(url) })
filePath.ext === '.json' ? JSON.parse(content) : parse(content)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

js-yaml’s load() method accepted a filename to display in error messages, which yaml’s parse() method does not. I guess this will be a slight DX reduction, but perhaps it’s acceptable?

If not, we could probably wrap the parse() block here to augment any error thrown with the filename as before.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice to have if you have many translation files I guess. And we could fix it in a way that also improves the DX for JSON as they don't have paths. Maybe something like:

for (const file of files) {
  const filePath = path.parse(file);
  if (!contentCollectionFileExtensions.includes(filePath.ext)) continue;
  const id = filePath.name;
  const url = new URL(file, i18nDir);
  const content = fs.readFileSync(url, 'utf-8');
  try {
    userTranslations[id] = (
      filePath.ext === '.json' ? JSON.parse(content) : parse(content)
    ) as i18nSchemaOutput;
  } catch (error) {
    const message = error instanceof Error ? error.message : String(error);
    throw new Error(`Failed to parse '${fileURLToPath(url)}':\n\n${message}`, {
      cause: error,
    });
  }
}

(but this would mean updating the test to check part of the message rather than the error type)

Comment thread packages/starlight/src/index.ts
@github-actions github-actions Bot added the 📚 docs Documentation website changes label Oct 6, 2026
@github-actions github-actions Bot added the 🌟 markdoc Changes to Starlight’s Markdoc package label Oct 6, 2026

@delucis delucis left a comment •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR with the latest releases of astro & co. to confirm js-yaml is now gone from the lock file. Hit a few small details while doing this, but otherwise the basics here seem to work well.

I did not update peer deps as well: everything should continue to work with the current peers, but people can get rid of some more deps by updating astro at the same time they update Starlight. This also lets us keep this a patch change.

if (!shouldTransformPath(fileURL, allowedPaths)) return;

const plugins = [satteriRtlCodeSupportPlugin()];
const plugins: HastPluginEntry[] = [satteriRtlCodeSupportPlugin()];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not directly related to the YAML chanegs, but a typing change required by updating astro.

relativePath ? `${relativePath}: ${issue.expected}` : issue.expected
);
} else if (issue.code === 'custom') {
} else if (issue.code === 'custom' && relativePath) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t have time to dig further, but seems like a change in Zod means that we now sometimes get a relativePath of "" for custom errors, which ends up getting added to the union display like SomeType | | SomeOtherType with the empty string joined by pipes as if it were a possible type in the union.

I’d still like to investigate further as IIUC the specific case we were hitting this was with the custom error here:

ctx.addIssue({
code: 'custom',
message:
`Found an \`autogenerate\` object with a \`label\`. Support for autogenerated sidebar groups was removed in Starlight v0.39.0.\n` +
`You should instead create a group with the desired \`label\` and an \`items\` array containing the autogenerate config:\n\n` +
`{\n` +
` label: '${config.label}',\n` +
` items: [{ autogenerate: ${JSON.stringify(
config.autogenerate,
// Hide empty attrs object that is automatically added by the schema default value.
(key, value: unknown) =>
key === 'attrs' &&
typeof value === 'object' &&
value !== null &&
Object.keys(value).length === 0
? undefined
: value,
' '
).replace(/\n\s*/g, ' ')} }]\n` +
`}`,
});

If I’m not mistaken, this message is not currently actually displayed, perhaps because it was silently swallowed by this part of the error map?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably the last sentence of this change.

Separately, an unrecognized_keys issue no longer aborts the schema it came from, so a strict object with an extra key and a bad value now reports both issues instead of just the first.

I guess we could add a comment to the change, maybe something like:

// Ignore custom issues on the value of the union option as they have no path
// associated with them to show in the expected type.
// Such issues come from `z.custom()` or `superRefine()` and since Zod 4.5,
// `superRefine()` runs even when a strict object has unknown keys.

@delucis
delucis marked this pull request as ready for review October 6, 2026 14:06

@HiDeoo HiDeoo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments but overall, the changes look good to me. Thanks for the PR 🙌

"@astrojs/starlight": patch
---

Reduces install size by ~400 KB by switching the YAML parser used internally

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Reduces install size by ~400 KB by switching the YAML parser used internally
Reduces install size by ~400 KB by switching the YAML parser used internally for projects using Astro 7.3.6 or later

Maybe to incentivize users to upgrade, we could mention the requirement?

};
const code =
source === 'config' ? JSON.stringify(correctTag, null, 2) : yaml.dump([correctTag]);
source === 'config' ? JSON.stringify(correctTag, null, 2) : stringify([correctTag]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
source === 'config' ? JSON.stringify(correctTag, null, 2) : stringify([correctTag]);
source === 'config'
? JSON.stringify(correctTag, null, 2)
: stringify([correctTag], { customTags: ['timestamp'] });

I think we need to use the same timestamp custom tag as Astro does.

Let's imagine the following invalid frontmatter:

head:
  - tag: meta
    attrs:
      name: date
    content: '2026-10-07'

You get the following error and hint:

  head.0: The `head` configuration includes a `meta` tag with `content` which is invalid HTML.
You should instead use a `content` attribute in the `attrs` object:

- tag: meta
  attrs:
    name: date
    content: 2026-10-07

But if you copy the hint, you get a second error due to the content now missing the quotes and Astro parsing it as a date.

  head.0.attrs.content**: **head.0.attrs.content: Did not match union.
> Expected type `"string"`, received `"object"`

Using { customTags: ['timestamp'] } shows the proper hint:

  head.0: The `head` configuration includes a `meta` tag with `content` which is invalid HTML.
You should instead use a `content` attribute in the `attrs` object:

- tag: meta
  attrs:
    name: date
    content: "2026-10-07"

I guess if we want a test, we could do something like:

test('suggests valid YAML for date-like `meta` content', () => {
  // Parse an invalid head config and get the raw error message.
  const result = HeadConfigSchema({ source: 'content' }).safeParse([
    { tag: 'meta', attrs: { name: 'date' }, content: '2026-10-07' },
  ]);

  // Extract the suggested YAML snippet from the error message.
  const suggestion = result.error?.issues[0]?.message.split('\n\n').at(-1);

  // Parse the suggestion like Astro parses frontmatter YAML.
  // https://github.com/withastro/astro/blob/88f0bd642b082274a57999bb9e74f89d7477e47e/packages/internal-helpers/src/yaml.ts#L4-L8
  const parsed: unknown = parse(suggestion ?? '', {
    customTags: ['timestamp'],
    merge: true,
    schema: 'core',
  });

  expect(parsed).toEqual([{ tag: 'meta', attrs: { name: 'date', content: '2026-10-07' } }]);
});

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, great find! I definitely completely missed that detail in the Astro migration PR. Will adjust.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lucky coincidence that I hit a similar issue in a plugin using the same dep, feels like déjà vu 😅

relativePath ? `${relativePath}: ${issue.expected}` : issue.expected
);
} else if (issue.code === 'custom') {
} else if (issue.code === 'custom' && relativePath) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably the last sentence of this change.

Separately, an unrecognized_keys issue no longer aborts the schema it came from, so a strict object with an extra key and a bad value now reports both issues instead of just the first.

I guess we could add a comment to the change, maybe something like:

// Ignore custom issues on the value of the union option as they have no path
// associated with them to show in the expected type.
// Such issues come from `z.custom()` or `superRefine()` and since Zod 4.5,
// `superRefine()` runs even when a strict object has unknown keys.

Comment on lines -38 to +36
filePath.ext === '.json'
? JSON.parse(content)
: yaml.load(content, { filename: fileURLToPath(url) })
filePath.ext === '.json' ? JSON.parse(content) : parse(content)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice to have if you have many translation files I guess. And we could fix it in a way that also improves the DX for JSON as they don't have paths. Maybe something like:

for (const file of files) {
  const filePath = path.parse(file);
  if (!contentCollectionFileExtensions.includes(filePath.ext)) continue;
  const id = filePath.name;
  const url = new URL(file, i18nDir);
  const content = fs.readFileSync(url, 'utf-8');
  try {
    userTranslations[id] = (
      filePath.ext === '.json' ? JSON.parse(content) : parse(content)
    ) as i18nSchemaOutput;
  } catch (error) {
    const message = error instanceof Error ? error.message : String(error);
    throw new Error(`Failed to parse '${fileURLToPath(url)}':\n\n${message}`, {
      cause: error,
    });
  }
}

(but this would mean updating the test to check part of the message rather than the error type)

Comment thread packages/starlight/src/index.ts

This branch has not been deployed

No deployments
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 📚 docs Documentation website changes 🌟 markdoc Changes to Starlight’s Markdoc package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants