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

Cannot access certain imports - Overriding components #860

Open
1 task done
TheOtterlord opened this issue Oct 11, 2023 · 5 comments
Open
1 task done

Cannot access certain imports - Overriding components #860

TheOtterlord opened this issue Oct 11, 2023 · 5 comments

Comments

@TheOtterlord
Copy link
Member

TheOtterlord commented Oct 11, 2023

What version of starlight are you using?

0.11.0

What version of astro are you using?

latest

What package manager are you using?

pnpm

What operating system are you using?

Linux

What browser are you using?

Firefox

Describe the Bug

Certain imports used by overridable components are not exported. utils is one I find I often need to access. Either because the builtin component uses it, or I need translation strings, etc.

List of missing exports I've noticed/needed so far.

  • utils
  • constants

Workaround

You can use a direct path to the node_modules folder to work around the missing exports.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-bd7al8?file=src%2FTest.astro

Participation

  • I am willing to submit a pull request for this issue.
@TheOtterlord
Copy link
Member Author

Adding to this, types are not exposed for the virtual module. The situation is a little different and will require documenting, but you can place this directive in your env.d.ts file to load types for the virtual modules. I feel this can be exposed via @astrojs/starlight/virtual or something like that, then we can document that you can add this to fix typescript.

/// <reference path="../node_modules/@astrojs/starlight/virtual.d.ts"/>

@delucis
Copy link
Member

delucis commented Oct 11, 2023

Thanks for the issue @TheOtterlord! This is partly intentional, although we should still find a way to support what you need to do.

Basically one of the aims of my refactors in #709 was to move as much logic as possible up out of components so we can keep utils/ as internal implementation details, so they’re easy to change without breaking people’s sites.

Translation strings

The one thing I decided was out of scope there was the translation strings for a couple of reasons:

  • Time is limited, so I just decided to put off that work 😁
  • Our current useTranslations() system is very basic and not really a proper solution for generic i18n needs. We’ve spoken about replacing it with a more battle-tested solution like i18next. Exposing our existing functions only to later change them seemed a shame if avoidable. (More about why a more robust system may be needed in “Lessons From Linguistics: i18n Best Practices for Front-End Developers” on the Shopify blog.)

One interim solution I like the sound of more is just to pre-generate the strings as part of generateRouteData() so that component props include a labels object which can be accessed directly, something like:

- import { useTranslations } from '../utils/translations';
- const t = useTranslations(Astro.props.locale);
- const label = t('page.editLink');
+ const label = Astro.props.labels['page.editLink'];

This would hide our implementation methods in return for a data-only model until we have time to investigate the more complex option of a full i18n system.

Other utils

Were there other things you need from utils? I’d love to apply a similar logic as much as possible: we offer you the data you need in props without needing further methods.

Constants

Maybe we take a similar approach and expose constants on the route data objects as well to avoid documenting the extra export? I agree that it’s useful to have this exposed — I already had to use _top without the constant in a docs example.

Types

Yeah, I realised this late in the day too and after trying a few things couldn’t type these automatically for people. I don’t know if we WANT those exposed though either for the same reason that the virtual module imports are kind of internal. What are you using them for? Maybe there’s another way to make sure the same data is available.

@HiDeoo
Copy link
Member

HiDeoo commented Oct 11, 2023

Just posted #861 and saw right now that it shares some points with this feedback so I can answer for me regarding the last point.

What are you using them for? Maybe there’s another way to make sure the same data is available.

So far for me, accessing virtual:starlight/user-config has been used for:

  • Figuring out the default locale to format some dates/currencies
  • Generating entryMeta when creating new routes
  • Check if some features are enabled or not like editLink, etc.

@TheOtterlord
Copy link
Member Author

Most of the time, requiring either utils or the virtual modules, it comes down to the existing implementation of a component that I copy into the new component. Sometimes some or all of these dependencies can be removed, but other times, I may only be overriding to add my own component, keeping the majority of the code the same.

I can replace certain component imports from a virtual module with the @astrojs/starlight/components/... import, but this would prevent any direct sub-components I've overridden from being included (e.g. I override PageSidebar and TableOfContents) requiring me to update those too (and depending on the scenario, still provide them as an override?).

pathWithBase is also used to override the SiteTitle link in the Astro docs. Maybe this is more of a niche scenario, but it's also something that could probably be exposed as a config option?

+1 on accessing the user config. This can be a really helpful module.

@TheOtterlord
Copy link
Member Author

One interim solution I like the sound of more is just to pre-generate the strings as part of generateRouteData() so that component props include a labels object which can be accessed directly, something like:

Yeah, I like this as a temporary solution. Obviously, it would be great to have a more complete solution in the future, so this is nice for now.

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

No branches or pull requests

3 participants