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

fix(v5): format adapter-reference.mdx #9423

Conversation

ArmandPhilippot
Copy link
Member

While I was reading a PR related to adapter-reference.mdx, I thought we could update the format to be consistent with configuration-reference.mdx and api-reference.mdx. Since v5 is set to replace the current version, I thought it might be more useful to update directly to that branch.

Description (required)

  • Add Type:, Default: and Since where appropriate
  • I moved some types from the section heading to Type:
  • Fix/Rename some types
  • Fix some line numbers to highlight in code blocks

A few hesitations:

  • Some properties are optional but are not explicitly marked with undefined as an accepted type. So in Type: I only put the accepted types (the property should not be added if it is undefined)
  • envGetSecret is in the Adapter features section but the type in the next branch of withastro/astro does not document it... So I obviously left it in, but I'm not sure of the type (I picked up the one in the sentence below) and I'm not sure if I should add Since (v5 I guess).
  • Some Defaults: that may not be explicit enough... I'll leave comments.

Related issues & labels (optional)

  • Suggested label: consistency/formatting

For Astro version: 5.0.0-beta.

This PR targets the 5.0.0-beta branch.

Copy link

netlify bot commented Sep 16, 2024

Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit df9c8d6
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/66e9bcfdb50ce700082ddfeb
😎 Deploy Preview https://deploy-preview-9423--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@@ -8,7 +8,7 @@ import { FileTree } from '@astrojs/starlight/components';

Astro is designed to make it easy to deploy to any cloud provider for SSR (server-side rendering). This ability is provided by __adapters__, which are [integrations](/en/reference/integrations-reference/). See the [SSR guide](/en/guides/on-demand-rendering/) to learn how to use an existing adapter.

## What is an adapter
## What is an Adapter
Copy link
Member Author

Choose a reason for hiding this comment

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

The next heading uses a capitalized adapter so... either both are capitalized or neither.

@@ -50,26 +50,30 @@ interface AstroAdapter {
exports?: string[];
args?: any;
adapterFeatures?: AstroAdapterFeatures;
supportedAstroFeatures?: AstroFeatureMap;
supportedAstroFeatures: AstroAdapterFeatureMap;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was renamed.

}

export type SupportsKind = 'unsupported' | 'stable' | 'experimental' | 'deprecated';
export type AdapterSupportsKind = 'unsupported' | 'stable' | 'experimental' | 'deprecated' | 'limited';
Copy link
Member Author

Choose a reason for hiding this comment

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

This was renamed.

Comment on lines +61 to +64
/**
* Determine the type of build output the adapter is intended for. Defaults to `server`;
*/
buildOutput?: 'static' | 'server';
Copy link
Member Author

Choose a reason for hiding this comment

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

buildOuput was missing.


export type AstroFeatureMap = {
export type AstroAdapterFeatureMap = {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was renamed.

Comment on lines +89 to +96
/**
* The adapter is able to support i18n domains
*/
i18nDomains?: AdapterSupport;
/**
* The adapter is able to support `getSecret` exported from `astro:env/server`
*/
envGetSecret?: AdapterSupport;
Copy link
Member Author

@ArmandPhilippot ArmandPhilippot Sep 16, 2024

Choose a reason for hiding this comment

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

i18nDomains was missing and I moved envGetSecret (while fixing the comment).

@@ -197,10 +211,21 @@ const response = await app.render(request);

##### `RenderOptions`

<p>

**Type:** `{addCookieHeader?: boolean; clientAddress?: string; locals?: object; routeData?: RouteData;}`
Copy link
Member Author

Choose a reason for hiding this comment

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

<p>

**Type:** `string`<br />
**Default:** `request[Symbol.for("astro.clientAddress")]`
Copy link
Member Author

@ArmandPhilippot ArmandPhilippot Sep 16, 2024

Choose a reason for hiding this comment

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

Not sure if this default is really useful...

<p>

**Type:** `RouteData`<br />
**Default:** `app.match(request)`
Copy link
Member Author

@ArmandPhilippot ArmandPhilippot Sep 16, 2024

Choose a reason for hiding this comment

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

It seems the default is the return type of app.match(request) and not app.match(request). But I wasn't sure about the value, maybe RouteData | undefined but it does not make sense as Default....

Copy link
Member

Choose a reason for hiding this comment

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

Should this now be IntegrationRouteData?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hadn't paid attention to the sentence below, so I understand your question... I checked again:

  • By switching back to the next branch: the type is still RouteData.
  • So I checked IntegrationRouteData to make sure it's not an alias: this is a smaller version of RouteData.

So I don't think so, RouteData type still exists and it seems to be the one expected. And I guess the sentence below remains valid: we provide a full RouteData object which is passed to integrationRouteData in its smaller form.
But a check from someone more informed on how it works may be useful, this is marked as **Advanced API**: you probably do not need to use this.

Copy link
Member

Choose a reason for hiding this comment

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

ok, then sounds good! Thank you!


<p>

**Type:** `(request: Request) => RouteData | undefined`
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Looks great, and thank you for jumping in to this! I just left a few comments!

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Looks great, and thank you for jumping in to this! I just left a few comments!

ArmandPhilippot and others added 3 commits September 17, 2024 19:15
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@sarah11918 sarah11918 added add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. consistency/formatting Standardizing without changing docs content e.g. indenting, lists etc. labels Sep 17, 2024
Copy link
Contributor

@casungo casungo left a comment

Choose a reason for hiding this comment

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

LGTM! Lots of small changes.

@sarah11918 sarah11918 merged commit 2d7bacf into withastro:5.0.0-beta Sep 17, 2024
5 checks passed
@ArmandPhilippot ArmandPhilippot deleted the fix/v5-format-adapter-reference.mdx branch September 17, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. consistency/formatting Standardizing without changing docs content e.g. indenting, lists etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants