-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
Implements top-level Astro + Astro.resolve #1556
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
import type { AstroComponentMetadata } from '../@types/astro'; | ||
import type { SSRResult } from '../@types/ssr'; | ||
import type { TopLevelAstro } from '../@types/astro-file'; | ||
|
||
import { pathToFileURL } from 'url'; | ||
import { valueToEstree } from 'estree-util-value-to-estree'; | ||
import * as astring from 'astring'; | ||
import shorthash from 'shorthash'; | ||
|
@@ -236,6 +238,47 @@ export async function renderComponent(result: SSRResult, displayName: string, Co | |
return `<astro-root uid="${astroId}">${html}</astro-root>`; | ||
} | ||
|
||
/** Create the Astro.fetchContent() runtime function. */ | ||
function createFetchContentFn(url: URL) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Semi-unrelated to this PR: I talked to Nate about possibly reorganizing the folders of the Bucket #1: manages SSR, Vite, and final HTML/CSS/JS output This would be in Bucket #2 which we’re calling “internal” right now. This gets weird if we import too many things for Vite to deal with, because sometimes it can’t tell what’s for the server and what’s for the client (import in SSR problem). Anyway, point of all that is I think this code belongs in here now (#2), and it was weird living in #1. I think some clearer separation will help us not only debug, but think about code execution as well (e.g. is this executing in Vite SSR or in Node?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @matthewp Do you have any thoughts on what these are named? Does this separation make sense? Or not really? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think those buckets make sense. But I also think the separation we currently have makes sense, internal is everything that astro files import. That should work whether it's coming from the Astro runtime or some other runtime (like the Deno one). But yeah, I don't have a complete idea of how to refactor everything, other than to say that how it's working right now seems fine to me, for now anyways. |
||
const fetchContent = (importMetaGlobResult: Record<string, any>) => { | ||
let allEntries = [...Object.entries(importMetaGlobResult)]; | ||
if (allEntries.length === 0) { | ||
throw new Error(`[${url.pathname}] Astro.fetchContent() no matches found.`); | ||
} | ||
return allEntries | ||
.map(([spec, mod]) => { | ||
// Only return Markdown files for now. | ||
if (!mod.frontmatter) { | ||
return; | ||
} | ||
return { | ||
content: mod.metadata, | ||
metadata: mod.frontmatter, | ||
file: new URL(spec, url), | ||
}; | ||
}) | ||
.filter(Boolean); | ||
}; | ||
return fetchContent; | ||
} | ||
|
||
export function createAstro(fileURLStr: string, site: string): TopLevelAstro { | ||
const url = pathToFileURL(fileURLStr); | ||
const fetchContent = createFetchContentFn(url) as unknown as TopLevelAstro['fetchContent']; | ||
return { | ||
// TODO I think this is no longer needed. | ||
isPage: false, | ||
site: new URL(site), | ||
fetchContent, | ||
resolve(...segments) { | ||
return segments.reduce( | ||
(u, segment) => new URL(segment, u), | ||
url | ||
).pathname | ||
} | ||
}; | ||
} | ||
|
||
export function addAttribute(value: any, key: string) { | ||
if (value == null || value === false) { | ||
return ''; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ import type { BuildResult } from 'esbuild'; | |
import type { ViteDevServer } from 'vite'; | ||
import type { AstroConfig, ComponentInstance, GetStaticPathsResult, Params, Props, Renderer, RouteCache, RouteData, RuntimeMode, SSRError } from '../@types/astro'; | ||
import type { SSRResult } from '../@types/ssr'; | ||
import type { Astro } from '../@types/astro-file'; | ||
import type { Astro, TopLevelAstro } from '../@types/astro-file'; | ||
import type { LogOptions } from '../logger'; | ||
|
||
import cheerio from 'cheerio'; | ||
|
@@ -80,30 +80,6 @@ async function resolveRenderers(viteServer: ViteDevServer, ids: string[]): Promi | |
return renderers; | ||
} | ||
|
||
/** Create the Astro.fetchContent() runtime function. */ | ||
function createFetchContentFn(url: URL) { | ||
const fetchContent = (importMetaGlobResult: Record<string, any>) => { | ||
let allEntries = [...Object.entries(importMetaGlobResult)]; | ||
if (allEntries.length === 0) { | ||
throw new Error(`[${url.pathname}] Astro.fetchContent() no matches found.`); | ||
} | ||
return allEntries | ||
.map(([spec, mod]) => { | ||
// Only return Markdown files for now. | ||
if (!mod.frontmatter) { | ||
return; | ||
} | ||
return { | ||
content: mod.metadata, | ||
metadata: mod.frontmatter, | ||
file: new URL(spec, url), | ||
}; | ||
}) | ||
.filter(Boolean); | ||
}; | ||
return fetchContent; | ||
} | ||
|
||
/** use Vite to SSR */ | ||
export async function ssr({ astroConfig, filePath, logging, mode, origin, pathname, route, routeCache, viteServer }: SSROptions): Promise<string> { | ||
try { | ||
|
@@ -128,6 +104,7 @@ export async function ssr({ astroConfig, filePath, logging, mode, origin, pathna | |
routeCache[route.component] = | ||
routeCache[route.component] || | ||
( | ||
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
await mod.getStaticPaths!({ | ||
paginate: generatePaginateFunction(route), | ||
rss: () => { | ||
|
@@ -158,35 +135,28 @@ export async function ssr({ astroConfig, filePath, logging, mode, origin, pathna | |
styles: new Set(), | ||
scripts: new Set(), | ||
/** This function returns the `Astro` faux-global */ | ||
createAstro: (props: Record<string, any>, slots: Record<string, any> | null) => { | ||
createAstro(AstroGlobal: TopLevelAstro, props: Record<string, any>, slots: Record<string, any> | null) { | ||
const site = new URL(origin); | ||
const url = new URL('.' + pathname, site); | ||
const canonicalURL = getCanonicalURL(pathname, astroConfig.buildOptions.site || origin); | ||
// Cast this type because the actual fetchContent implementation relies on import.meta.globEager | ||
const fetchContent = createFetchContentFn(filePath) as unknown as Astro['fetchContent']; | ||
const canonicalURL = getCanonicalURL('.' + pathname, astroConfig.buildOptions.site || origin); | ||
|
||
return { | ||
isPage: true, | ||
site, | ||
__proto__: AstroGlobal, | ||
props, | ||
request: { | ||
canonicalURL, | ||
params: {}, | ||
url | ||
}, | ||
props, | ||
fetchContent, | ||
slots: Object.fromEntries( | ||
Object.entries(slots || {}).map(([slotName]) => [slotName, true]) | ||
), | ||
// Only temporary to get types working. | ||
resolve(_s: string) { | ||
throw new Error('Astro.resolve() is not currently supported in next.'); | ||
} | ||
}; | ||
) | ||
} as unknown as Astro; | ||
}, | ||
_metadata: { renderers }, | ||
}; | ||
|
||
let html = await renderPage(result, Component, {}, null); | ||
let html = await renderPage(result, Component, pageProps, null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! We get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this back because one of the astro-global tests was using dynamic routes. So I didn't make any changes that fixed this AFAIK. |
||
|
||
// 4. modify response | ||
if (mode === 'development') { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,3 @@ | ||
/** | ||
* UNCOMMENT: add Astro.* global | ||
|
||
import { expect } from 'chai'; | ||
import cheerio from 'cheerio'; | ||
import { loadFixture } from './test-utils.js'; | ||
|
@@ -32,15 +29,16 @@ describe('Astro.*', () => { | |
it('Astro.request.canonicalURL', async () => { | ||
// given a URL, expect the following canonical URL | ||
const canonicalURLs = { | ||
'/': 'https://mysite.dev/blog/index.html', | ||
'/post/post': 'https://mysite.dev/blog/post/post/index.html', | ||
'/posts/1': 'https://mysite.dev/blog/posts/index.html', | ||
'/posts/2': 'https://mysite.dev/blog/posts/2/index.html', | ||
'/index.html': 'https://mysite.dev/blog/', | ||
'/post/post/index.html': 'https://mysite.dev/blog/post/post/', | ||
'/posts/1/index.html': 'https://mysite.dev/blog/posts/', | ||
'/posts/2/index.html': 'https://mysite.dev/blog/posts/2/', | ||
}; | ||
|
||
for (const [url, canonicalURL] of Object.entries(canonicalURLs)) { | ||
const result = await fixture.readFile(url); | ||
const $ = cheerio.load(result.contents); | ||
const html = await fixture.readFile(url); | ||
|
||
const $ = cheerio.load(html); | ||
expect($('link[rel="canonical"]').attr('href')).to.equal(canonicalURL); | ||
} | ||
}); | ||
|
@@ -54,16 +52,7 @@ describe('Astro.*', () => { | |
it('Astro.resolve in development', async () => { | ||
const html = await fixture.readFile('/resolve/index.html'); | ||
const $ = cheerio.load(html); | ||
expect($('img').attr('src')).to.equal('/_astro/src/images/penguin.png'); | ||
expect($('#inner-child img').attr('src')).to.equal('/_astro/src/components/nested/images/penguin.png'); | ||
}); | ||
|
||
it('Astro.resolve in the build', async () => { | ||
const html = await fixture.readFile('/resolve/index.html'); | ||
const $ = cheerio.load(html); | ||
expect($('img').attr('src')).to.equal('/blog/_astro/src/images/penguin.png'); | ||
expect($('img').attr('src')).to.equal('/src/images/penguin.png'); | ||
expect($('#inner-child img').attr('src')).to.equal('/src/components/nested/images/penguin.png'); | ||
}); | ||
}); | ||
*/ | ||
|
||
it.skip('is skipped', () => {}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎉 🎉 🎉 |
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
--- | ||
const title = 'My App'; | ||
import '../components/my-element.js'; | ||
const title = 'My App'; | ||
--- | ||
|
||
<html> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
--- | ||
const title = 'My App'; | ||
import '../components/my-element.js'; | ||
const title = 'My App'; | ||
--- | ||
|
||
<html> | ||
|
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.
If we don’t like this rule we can change it globally! Up to you
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 the rule is fine, but just not in this case when using a Map with has followed by get. If there were a way to turn off just this one use-case that would be fine, but otherwise its not a big deal to add the comment.