Skip to content

Conversation

@bholmesdev
Copy link
Contributor

Motivation: Hackathon participants were confused how integrations should be built. This PR introduces detailed docs and examples for each API hook!

@netlify
Copy link

netlify bot commented Apr 27, 2022

Deploy Preview for astro-docs-2 ready!

Name Link
🔨 Latest commit 6420a33
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/626c23aa95a4980008b01c6f
😎 Deploy Preview https://deploy-preview-388--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 settings.

Copy link
Contributor

@aFuzzyBear aFuzzyBear left a comment

Choose a reason for hiding this comment

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

Much needed, a great improvement on the API, thanks ben this would really help alot of folk out,

@roblevintennis
Copy link

roblevintennis commented Apr 28, 2022

TL;DR my working commit is here:
https://github.com/AgnosticUI/agnosticui/pull/207/files


It also occurs to me that a nice follow up to this PR would be an example of setting up a dev env for local testing of said integration. For example, should I create a /integration_test that's a literal astro project? Then do I do npm pack and point to that tarball? Where exactly should the integration files go and what are the conventions there?

In terms of building the integration the official integrations have:

"build": "astro-scripts build \"src/**/*.ts\" && tsc"

But we won't have access to that. Is there a recommended local dev setup for this? Just a boiler-plate tsc based setup like:

npm install typescript --save-dev
npx tsc # setup as a package.json script perhaps

tsconfig.json

{
  "include": ["src"],
  "compilerOptions": {
    "declaration": true,
    "strict": true,
    "moduleResolution": "node",
    "esModuleInterop": true,
    "skipLibCheck": true,
    "forceConsistentCasingInFileNames": true,
    "allowJs": true,
    "module": "ES2020",
    "outDir": "./dist",
    "target": "ES2020"
  }
}

Then in my package.json

  "scripts": {
    "build": "npx tsc"
  },

This generates dist/index.js and dist/index.d.ts which seems correct. Please advise.

Update: Above setup seems to work...so I'm just going to document here (temporarily) everything that worked for me in case other hackathon participants are stuck and/or we want to provide a more complete hello world example similar to what I've done here. Will link to my repo once I've actually pushed all this up ;)


Here's what I put in my integrations/astro/agnostic-astro/src/index.ts (about the simplest integration one could think of!):

import type { AstroIntegration } from 'astro';
export default function createPlugin(): AstroIntegration {
  return {
    name: '@astrojs/agnostic-astro',
    hooks: {
      'astro:config:setup': ({ injectScript }) => {
        // Ideally, we only import the common.min.css once and before build-time
        injectScript('page', `import 'agnostic-css/public/css-dist/common.min.css';`);
      },
    },
  };
}

Here's the integrations package.json:

{
  "name": "@astrojs/agnostic-astro",
  "description": "Astro + AgnosticUI integration",
  "version": "0.0.1",
  "type": "module",
  "types": "./dist/index.d.ts",
  "author": "Rob Levin <roblevinillustration@gmail.com>",
  "license": "Apache-2.0",
  "repository": {
    "type": "git",
    "url": "https://github.com/AgnosticUI/agnosticui.git"
  },
  "keywords": [
    "astro-component",
    "agnosticui",
    "agnostic-astro"
  ],
  "bugs": "https://github.com/withastro/astro/issues",
  "homepage": "https://astro.build",
  "files": [
    "src/index.ts",
    "dist/index.js",
    "dist/index.d.ts"
  ],
  "exports": {
    ".": "./dist/index.js"
  },
  "scripts": {
    "build": "npx tsc"
  },
  "dependencies": {
    "agnostic-astro": "latest"
  },
  "devDependencies": {
    "astro": "latest",
    "typescript": "^4.6.3"
  }
}

To test this locally I build tsc as described above and then utilize npm pack which generates and exact npm package tarball replica:

npm run build && npm pack

Then I created a create astro (minimal) in integrations/astro/agnostic-astro/test just to test this. Here's that test app's astro.config.mjs:

import { defineConfig } from 'astro/config';
import agnosticAstro from '@astrojs/agnostic-astro';

export default defineConfig({
  integrations: [agnosticAstro()]
});

Nothing surprising. To use the integration locally I npm install the previously built tarball and run dev like this:

npm i ../astrojs-agnostic-astro-0.0.1.tgz && npm run dev

Here's what the package.json will look like after you've installed that tarball (you could use npm link too but I've found pack much more reliable):

{
  "name": "@example/minimal",
  "version": "0.0.1",
  "private": true,
  "scripts": {
    "dev": "astro dev",
    "start": "astro dev",
    "build": "astro build",
    "preview": "astro preview"
  },
  "devDependencies": {
    "astro": "^1.0.0-beta.19"
  },
  "dependencies": {
    "@astrojs/agnostic-astro": "file:../astrojs-agnostic-astro-0.0.1.tgz"
  }
}

And when I hit the test app I just want to see my injectedScript common.min.css styles were in fact applied (that defines all the css custom properties fwiw):

image

@FredKSchott
Copy link
Member

This is great! The only big feedback that I'd have is to add an example below each hook that just shows all of the options, so that the reader understands what “config” option means.
Example:

## astro:config:setup

When it’s run: ...
Use case: ...

'''
'astro:config:setup': ({ 
  config,
  command,
  updateConfig,
  addRenderer,
  injectScript,
}) => {
  // ...
},
'''

Open to other ideas here as well, as long as it solves the "what does “config” option mean?" problem.


### astro:config:setup

**When it's run:** on initialization, before either the [Vite](https://vitejs.dev/config/) or [Astro config](/en/reference/configuration-reference/) have resolved.
Copy link
Member

@FredKSchott FredKSchott Apr 28, 2022

Choose a reason for hiding this comment

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

Nit: can you follow the format that we use on https://deploy-preview-388--astro-docs-2.netlify.app/en/reference/configuration-reference/

- **When it's run:** ...
- 
- **Use case:** ...
+ **When it's run:** ...  (note: 2 spaces at the end for a newline)
+ **Use case:** ...

Also, "When it's run" and "Use case" are almost in two different voices (conversational vs descriptive). Stylistically, I like to shorten these so that they are easier for the readers brain to "key" to when scanning the page. Example:

**Lifecycle:** ... (alt: "Stage")
**Purpose:** ...

Finally, a non-blocking suggestion: it could be interesting to do something like what Rollup does with their hooks docs (ex: https://rollupjs.org/guide/en/#buildend). This "type" item would actually also solve my comment I made about wanting to see an example with all of the options so that users knew what "config" option was referring to:

Type: `({config, command, updateConfig, addRenderer, injectScript}) => void`
Previous Hook: None
Next Hook: `astro:config:done` (clickable link to it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried lifecycle but it didn't make the most sense in context. I landed on simply "when" and "why!"

Copy link
Contributor

@aFuzzyBear aFuzzyBear left a comment

Choose a reason for hiding this comment

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

Its nae mare skewwhiff

@bholmesdev
Copy link
Contributor Author

@roblevintennis Don't wanna lose your detailed suggestion! I'd suggest moving this to a docs issue so we can follow-up here. I'll look into this myself for some best practices with pnpm 😁


**Type:** `(newConfig: Record<string, any>) => void;`

A callback function to update the user-supplied [Astro config](/en/reference/configuration-reference/). Any config you provide **will be merged with the user config + other integration config updates,** so you are free to omit keys!
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A callback function to update the user-supplied [Astro config](/en/reference/configuration-reference/). Any config you provide **will be merged with the user config + other integration config updates,** so you are free to omit keys!
Callback function to update the user-supplied [Astro config](/en/reference/configuration-reference/). Any configurations you provide **will be merged with the user config + other integration config updates,** so you are free to omit keys!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've been starting each section with "a [blank]" for readability, and I wouldn't want to change this now 😕


**Type:** `(stage: InjectedScriptStage, content: string) => void;`

A callback function to inject a string of JavaScript content onto every page.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A callback function to inject a string of JavaScript content onto every page.
Callback function to inject a string of JavaScript content onto every page.

Copy link
Contributor

@aFuzzyBear aFuzzyBear left a comment

Choose a reason for hiding this comment

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

If possible could we get this merged before Close of Play?

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

Successfully merging this pull request may close these issues.

7 participants