-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(assets): document remote utilities #11158
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I wasn't aware of these utilities!
However, I think it's best to stay consistent with the other API references, so I would say we should:
- use the API reference notation instead of code blocks for the type, which also allow us to add a
Since
in the same block if new utilities are added later - move the
matchProtocol
example under the corresponding section - and, yes, if we can think of some use cases, I think it would be nice to have an example for each utility to add more colours/help users (but not mandatory... we can always add one later if a user reports that they have trouble understanding how to use it)
The first type is a bit long but I think it's still suitable:
Before | After |
---|---|
![]() |
![]() |
(Obviously my example is neither pretty nor correct because I edited it quickly in the browser... 😅 )
Ideally, I think we should also update the hooks section with API notation. We've done this on other API reference pages, but this page hasn't been updated yet. However, we can do that later! I'm just saying this for context (ie. we shouldn't use the Hooks section as an example of the correct format).
```ts | ||
import { matchProtocol } from "astro/assets/utils"; | ||
|
||
matchProtocol(new URL("https://example.com"), "https"); // returns true | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should move this example under the right section, but maybe we could replace it with the list of available imports like we do in astro:assets
for example:
```ts | |
import { matchProtocol } from "astro/assets/utils"; | |
matchProtocol(new URL("https://example.com"), "https"); // returns true | |
``` | |
```ts | |
import { | |
isRemoteAllowed, | |
matchHostname, | |
matchPathname, | |
matchPattern, | |
matchPort, | |
matchProtocol | |
} from "astro/assets/utils"; | |
``` |
Description (required)
This is a follow-up PR of the code PR withastro/astro#13355
I added the new remote pattern utilities at the end of the page. I used the description that we have in the code as a starting point, and of course we can change them.
Questions:
As a tangent, Astro exposes more functions from the specifier
astro/assets/utils
that we don't document. However, I believe that not all functions are meant to be exposed to userland. I want to check with the team first, and then we can do follow-up PRs to add the missing functions.