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

Expose FS translation system to plugins #2578

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

Conversation

HiDeoo
Copy link
Member

@HiDeoo HiDeoo commented Nov 5, 2024

Description

This plugin exposes the file system translation system to Starlight plugins throught the setup hook using a new useTranslations() callback function.

This change requires the addition of a new plugin hook (currently named init, I picked the first thing coming to mind to get started) which now exposes the injectTranslations() callback function (which was previously available in the setup hook).

The documentation so far is only a rough draft to get an idea of everything changed.

Remaining tasks

  • Restore the docs/astro.config.mjs file
  • Restore the docs/package.json file
  • Restore the docs/src/content/i18n/zh-CN.json file
  • Remove the entire packages/demo-plugin/ directory
  • Run pnpm i to remove mention of the starlight-demo-plugin package in the lockfile
  • Remove all remaining // TODO(HiDeoo) comments (if any)
  • Order the changeset in the release PR so that .changeset/polite-fishes-remain.md comes first (Deprecates the Starlight plugin `setup` hook in favor of the new `config:setup` hook which provides the same functionality.)

Copy link

changeset-bot bot commented Nov 5, 2024

🦋 Changeset detected

Latest commit: e80caa4

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

This PR includes changesets to release 2 packages
Name Type
@astrojs/starlight Minor
@astrojs/starlight-docsearch 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

@github-actions github-actions bot added 📚 docs Documentation website changes 🌟 core Changes to Starlight’s main package labels Nov 5, 2024
Copy link

netlify bot commented Nov 5, 2024

Deploy Preview for astro-starlight ready!

Name Link
🔨 Latest commit e80caa4
🔍 Latest deploy log https://app.netlify.com/sites/astro-starlight/deploys/677f98d4a8395d000842ed30
😎 Deploy Preview https://deploy-preview-2578--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: 100 (🟢 up 11 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

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

@@ -152,7 +151,7 @@ export function getStarlightEcConfigPreprocessor({
},
...otherStyleOverrides,
},
getBlockLocale: ({ file }) => pathToLocale(file.path, { starlightConfig, astroConfig }),
getBlockLocale: ({ file }) => pathToLang(file.path, { astroConfig, starlightConfig }),
Copy link
Member Author

Choose a reason for hiding this comment

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

Expressive Code technically expects a Starlight language here so I changed it but this is not a bug fix (no changeset) as Expressive Code lower-cases the language identifier anyway.

@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Nov 5, 2024

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

Locale File Note
en guides/i18n.mdx Source changed, localizations will be marked as outdated.
en reference/plugins.md Source changed, localizations will be marked as outdated.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

This looks great @HiDeoo! Left some suggestions/questions/discussions, but they’re honestly all around the “outside” of the core functionality or on the docs. I think overall this seems very solid 🚀

.changeset/large-balloons-compete.md Outdated Show resolved Hide resolved
packages/starlight/integrations/expressive-code/index.ts Outdated Show resolved Hide resolved
// strings. We let TypeScript merge them into a single union type so that plugins with a TypeScript
// configuration preventing `UserI18nKeys` to be properly inferred can still get auto-completion
// for built-in UI strings.
export type I18nKeys = keyof BuiltInStrings | UserI18nKeys | keyof StarlightApp.I18n;
Copy link
Member

Choose a reason for hiding this comment

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

Ahhhhhh, nice fix! 🧠

docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
@HiDeoo
Copy link
Member Author

HiDeoo commented Nov 8, 2024

Thanks for the review 🙌

Holding off on the suggested changes so I can tackle them all at once when we have a clearer idea of what we want to do regarding the name.

HiDeoo and others added 12 commits November 9, 2024 17:46
* main: (27 commits)
  i18n(ko-KR): update `site-search.mdx`
  [ci] release (withastro#2590)
  Support passing more options to the DocSearch component (withastro#2589)
  [ci] release (withastro#2587)
  Add changeset for withastro#2252 (withastro#2588)
  [ci] format
  Update dev dependencies (withastro#2582)
  Build performance optimizations for projects with large sidebars (withastro#2252)
  Fix a11y CI workflow (withastro#2503)
  Update `astro-expressive-code` to v0.38 (withastro#2551)
  Added social icon for Nostr (withastro#2579)
  [ci] format
  docs(showcase): add docs.reactbricks.com to showcase (withastro#2586)
  i18n(ja): Update site-search.mdx (withastro#2577)
  i18n(ja): Update pages.mdx (withastro#2576)
  i18n(fr): Update `reference/plugins.mdx` from withastro#2549 (withastro#2574)
  [ci] format
  docs: update showcase-sites.astro (withastro#2562)
  Throw an error if a showcase image does not have the required dimensions (withastro#2573)
  i18n(ja): Update frontmatter.md (withastro#2566)
  ...
@HiDeoo
Copy link
Member Author

HiDeoo commented Nov 11, 2024

Updated the branch with the changes discussed above, hope I did not forget anything:

  • Adds the config:setup hook and deprecate the setup one
    • I added a few tests to ensure we make sure to totally remove setup before v1
  • Renames the new hook to i18n:setup
  • Cleans up and renames the new path utility to absolutePathToLang
    • It's fairly easy to add some checks to ensure the paths are the ones we expect for a document in a content collection altho, this would fail as soon as something like import { Content } from '../../something.md' is used (this would even fail with our EmptyMarkdown file).
    • Should we explicitly mention that if it's not matching a configured locale, it's falling back to the default locale?
  • Adds a test to ensure we migrate @astrojs/starlight-docsearch to the config:setup hook before v1
  • Adds a new HookParameters utility type and its documentation
    • A lot of plugin are already doing something like Parameters<StarlightPlugin['hooks']['setup']>['0']['something'] but now this would be even longer with the addition of a nullable so the new type should help with that like in Astro.
  • Updates starlight-links-validator to a new version where I refactored some very old legacy types to avoid type errors
  • Updates the useTranslations() documentation & example to mention it's a BCP-47 tag and to use a region subtag in the example

I have a question: this PR adds multiple changesets but I think .changeset/polite-fishes-remain.md should come first. Altho, I don't know if there is a way to enforce that from this PR, right?

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

I think this looks good! Left a few small notes, but nothing significant jumped out to me — just cleaning up the demo plugin and removing any other TODOs.

I have a question: this PR adds multiple changesets but I think .changeset/polite-fishes-remain.md should come first. Altho, I don't know if there is a way to enforce that from this PR, right?

Good question! I’m not 100% sure how Changesets orders them beyond the major/minor/patch subheadings. Not sure if it’s chronological by commit time or something else. I’d guess we should just tweak the CHANGELOG in the release PR when the time comes.

docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
Comment on lines 185 to 197
test.runIf(pkg.version[0] === '1')(
'removes the deprecated `setup` hook in the DocSearch plugin with Starlight v1',
async () => {
const docSearchPlugin = starlightDocSearch({
appId: 'YOUR_APP_ID',
apiKey: 'YOUR_SEARCH_API_KEY',
indexName: 'YOUR_INDEX_NAME',
});
expect(docSearchPlugin.hooks.setup).not.toBeDefined();
expect(docSearchPlugin.hooks['config:setup']).toBeDefined();
}
);
});
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I would be happy to just change the DocSearch plugin right away, bumping the minimum supported Starlight version, and release a new minor. Can also happen in a follow-up PR though as we can release a new plugin version after this Starlight minor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, one more changeset it is 😅

Comment on lines 67 to 69
const integrations = pluginResult.integrations;
const useTranslations = pluginResult.useTranslations;
const absolutePathToLang = pluginResult.absolutePathToLang;
Copy link
Member

Choose a reason for hiding this comment

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

Should we refactor this to this?

Suggested change
const integrations = pluginResult.integrations;
const useTranslations = pluginResult.useTranslations;
const absolutePathToLang = pluginResult.absolutePathToLang;
const { integrations, useTranslations, absolutePathToLang } = pluginResult;

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, please ^^ I imagine I just duplicated the lines at the early stage of the code and missed the potential refactor later on 😞

@delucis delucis marked this pull request as ready for review January 8, 2025 18:59
@delucis delucis added the 🌟 minor Change that triggers a minor release label Jan 8, 2025
@github-actions github-actions bot added the 🌟 docsearch Changes to Starlight’s DocSearch plugin label Jan 9, 2025
@HiDeoo
Copy link
Member Author

HiDeoo commented Jan 9, 2025

I’d guess we should just tweak the CHANGELOG in the release PR when the time comes.

Makes sense, that was also my guess, thanks. I added a new todo to the PR body to remember about that.

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Perfect! Just noticed one detail with the DocSearch update.

packages/docsearch/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Perfect! OK, should be ready to go I think! I’m thinking of trying to get Astro.locals ready too, so that these two changes breaking plugins can go out together. I think we’re almost there.

@delucis delucis added this to the v0.31 milestone Jan 9, 2025
@HiDeoo
Copy link
Member Author

HiDeoo commented Jan 9, 2025

I’m thinking of trying to get Astro.locals ready too, so that these two changes breaking plugins can go out together. I think we’re almost there.

Makes sense 👍 I already gathered some feedback/concerns regarding the route data PR to publish when it's ready for review, really excited about this one 🤩

@delucis delucis modified the milestones: v0.31, v0.32 Jan 10, 2025
@trueberryless
Copy link
Contributor

I have a question: this PR adds multiple changesets but I think .changeset/polite-fishes-remain.md should come first. Altho, I don't know if there is a way to enforce that from this PR, right?

I'm not 100% sure, but I think that changesets just reads the files alphabetically, which means changing the filename from polite-fishes-remain.md to e.g. attractive-fishes-remain.md should do the trick.

Altho I have to say, I personally would also double check it in the ci PR later, so leaving the ToDo is also my recommendation. Nonetheless, trying it out would be interesting 🤔

Reference: In one of my releases you can see that in the patch section it works that way, but that's actually not a trustworthy reference because it could have many other reasons...

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 🌟 docsearch Changes to Starlight’s DocSearch plugin 🌟 minor Change that triggers a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants