From b463ddb3ce43641536dee4413ac5bb8ebcbdf608 Mon Sep 17 00:00:00 2001 From: Tony Sullivan Date: Thu, 12 May 2022 16:04:01 +0000 Subject: [PATCH] Resolve components by module ID during compilation (#3300) * WIP: adding test coverage * test fixes * moving the shared lib up a directory to reproduce the bug * fix: transform with the module ID instead of parsing the filepath * adding the shared lib to the workspaces list * fix: client-only assets now get the full URL from vite * why is this needed for windows? * WIP: using /@fs to handle windows filepaths * fix: remove /@fs from hoisted script imports * nit: removing unused imports * fix: strip off the path root when mapping client:only styles * had to reverse the `/@fs` handling to work on windows and unix * chore: adding comments to explain the fix * chore: adding changeset --- .changeset/proud-cups-brush.md | 5 + package.json | 3 +- packages/astro/src/core/build/internal.ts | 20 +-- packages/astro/src/core/build/static-build.ts | 2 +- .../core/build/vite-plugin-hoisted-scripts.ts | 7 +- .../astro/src/vite-plugin-astro/compile.ts | 41 +++-- packages/astro/src/vite-plugin-astro/index.ts | 20 ++- packages/astro/test/component-library.test.js | 157 ++++++++++++++++++ .../component-library-shared/Button.astro | 7 + .../component-library-shared/Counter.css | 11 ++ .../component-library-shared/Counter.jsx | 20 +++ .../component-library-shared/Counter.svelte | 33 ++++ .../CounterWrapper.astro | 7 + .../component-library-shared/HelloReact.jsx | 5 + .../component-library-shared/README.md | 5 + .../component-library-shared/package.json | 20 +++ .../component-library/astro.config.mjs | 8 + .../fixtures/component-library/package.json | 14 ++ .../src/pages/internal-hydration.astro | 17 ++ .../src/pages/with-astro.astro | 15 ++ .../src/pages/with-react.astro | 7 + pnpm-lock.yaml | 24 +++ 22 files changed, 407 insertions(+), 41 deletions(-) create mode 100644 .changeset/proud-cups-brush.md create mode 100644 packages/astro/test/component-library.test.js create mode 100644 packages/astro/test/fixtures/component-library-shared/Button.astro create mode 100644 packages/astro/test/fixtures/component-library-shared/Counter.css create mode 100644 packages/astro/test/fixtures/component-library-shared/Counter.jsx create mode 100644 packages/astro/test/fixtures/component-library-shared/Counter.svelte create mode 100644 packages/astro/test/fixtures/component-library-shared/CounterWrapper.astro create mode 100644 packages/astro/test/fixtures/component-library-shared/HelloReact.jsx create mode 100644 packages/astro/test/fixtures/component-library-shared/README.md create mode 100644 packages/astro/test/fixtures/component-library-shared/package.json create mode 100644 packages/astro/test/fixtures/component-library/astro.config.mjs create mode 100644 packages/astro/test/fixtures/component-library/package.json create mode 100644 packages/astro/test/fixtures/component-library/src/pages/internal-hydration.astro create mode 100644 packages/astro/test/fixtures/component-library/src/pages/with-astro.astro create mode 100644 packages/astro/test/fixtures/component-library/src/pages/with-react.astro diff --git a/.changeset/proud-cups-brush.md b/.changeset/proud-cups-brush.md new file mode 100644 index 0000000000..0927ec9213 --- /dev/null +++ b/.changeset/proud-cups-brush.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Resolve .astro components by module ID to support the use of Astro + framework components in an NPM package diff --git a/package.json b/package.json index 0eaa817d03..a3efb0c61c 100644 --- a/package.json +++ b/package.json @@ -33,8 +33,7 @@ "examples/component/packages/*", "scripts", "smoke/*", - "packages/astro/test/fixtures/builtins/packages/*", - "packages/astro/test/fixtures/builtins-polyfillnode", + "packages/astro/test/fixtures/component-library-shared", "packages/astro/test/fixtures/custom-elements/my-component-lib", "packages/astro/test/fixtures/static build/pkg" ], diff --git a/packages/astro/src/core/build/internal.ts b/packages/astro/src/core/build/internal.ts index 9edde85f34..c6f98a4fa5 100644 --- a/packages/astro/src/core/build/internal.ts +++ b/packages/astro/src/core/build/internal.ts @@ -1,8 +1,7 @@ -import type { AstroConfig, RouteData } from '../../@types/astro'; import type { RenderedChunk } from 'rollup'; import type { PageBuildData, ViteID } from './types'; -import { fileURLToPath } from 'url'; +import { prependForwardSlash } from '../path.js'; import { viteID } from '../util.js'; export interface BuildInternals { @@ -80,17 +79,16 @@ export function trackPageData( export function trackClientOnlyPageDatas( internals: BuildInternals, pageData: PageBuildData, - clientOnlys: string[], - astroConfig: AstroConfig + clientOnlys: string[] ) { for (const clientOnlyComponent of clientOnlys) { - const coPath = viteID(new URL('.' + clientOnlyComponent, astroConfig.root)); let pageDataSet: Set; - if (internals.pagesByClientOnly.has(coPath)) { - pageDataSet = internals.pagesByClientOnly.get(coPath)!; + // clientOnlyComponent will be similar to `/@fs{moduleID}` + if (internals.pagesByClientOnly.has(clientOnlyComponent)) { + pageDataSet = internals.pagesByClientOnly.get(clientOnlyComponent)!; } else { pageDataSet = new Set(); - internals.pagesByClientOnly.set(coPath, pageDataSet); + internals.pagesByClientOnly.set(clientOnlyComponent, pageDataSet); } pageDataSet.add(pageData); } @@ -115,8 +113,10 @@ export function* getPageDatasByClientOnlyChunk( const pagesByClientOnly = internals.pagesByClientOnly; if (pagesByClientOnly.size) { for (const [modulePath] of Object.entries(chunk.modules)) { - if (pagesByClientOnly.has(modulePath)) { - for (const pageData of pagesByClientOnly.get(modulePath)!) { + // prepend with `/@fs` to match the path used in the compiler's transform() call + const pathname = `/@fs${prependForwardSlash(modulePath)}`; + if (pagesByClientOnly.has(pathname)) { + for (const pageData of pagesByClientOnly.get(pathname)!) { yield pageData; } } diff --git a/packages/astro/src/core/build/static-build.ts b/packages/astro/src/core/build/static-build.ts index 6150dd9954..8b55e83bf1 100644 --- a/packages/astro/src/core/build/static-build.ts +++ b/packages/astro/src/core/build/static-build.ts @@ -56,7 +56,7 @@ export async function staticBuild(opts: StaticBuildOptions) { // Track client:only usage so we can map their CSS back to the Page they are used in. const clientOnlys = Array.from(metadata.clientOnlyComponentPaths()); - trackClientOnlyPageDatas(internals, pageData, clientOnlys, astroConfig); + trackClientOnlyPageDatas(internals, pageData, clientOnlys); const topLevelImports = new Set([ // Any component that gets hydrated diff --git a/packages/astro/src/core/build/vite-plugin-hoisted-scripts.ts b/packages/astro/src/core/build/vite-plugin-hoisted-scripts.ts index a355fb282b..8e4872e0c7 100644 --- a/packages/astro/src/core/build/vite-plugin-hoisted-scripts.ts +++ b/packages/astro/src/core/build/vite-plugin-hoisted-scripts.ts @@ -25,7 +25,12 @@ export function vitePluginHoistedScripts( if (virtualHoistedEntry(id)) { let code = ''; for (let path of internals.hoistedScriptIdToHoistedMap.get(id)!) { - code += `import "${path}";`; + let importPath = path; + // `/@fs` is added during the compiler's transform() step + if (importPath.startsWith('/@fs')) { + importPath = importPath.slice('/@fs'.length); + } + code += `import "${importPath}";`; } return { code, diff --git a/packages/astro/src/vite-plugin-astro/compile.ts b/packages/astro/src/vite-plugin-astro/compile.ts index f17695047e..7301d47342 100644 --- a/packages/astro/src/vite-plugin-astro/compile.ts +++ b/packages/astro/src/vite-plugin-astro/compile.ts @@ -34,16 +34,25 @@ function safelyReplaceImportPlaceholder(code: string) { const configCache = new WeakMap(); -async function compile( - config: AstroConfig, - filename: string, - source: string, - viteTransform: TransformHook, - opts: { ssr: boolean } -): Promise { +interface CompileProps { + config: AstroConfig; + filename: string; + moduleId: string; + source: string; + ssr: boolean; + viteTransform: TransformHook; +} + +async function compile({ + config, + filename, + moduleId, + source, + ssr, + viteTransform, +}: CompileProps): Promise { const filenameURL = new URL(`file://${filename}`); const normalizedID = fileURLToPath(filenameURL); - const pathname = filenameURL.pathname.slice(config.root.pathname.length - 1); let rawCSSDeps = new Set(); let cssTransformError: Error | undefined; @@ -52,7 +61,8 @@ async function compile( // use `sourcemap: "both"` so that sourcemap is included in the code // result passed to esbuild, but also available in the catch handler. const transformResult = await transform(source, { - pathname, + // For Windows compat, prepend the module ID with `/@fs` + pathname: `/@fs${prependForwardSlash(moduleId)}`, projectRoot: config.root.toString(), site: config.site ? new URL(config.base, config.site).toString() : undefined, sourcefile: filename, @@ -86,7 +96,7 @@ async function compile( lang, id: normalizedID, transformHook: viteTransform, - ssr: opts.ssr, + ssr, }); let map: SourceMapInput | undefined; @@ -131,13 +141,8 @@ export function invalidateCompilation(config: AstroConfig, filename: string) { } } -export async function cachedCompilation( - config: AstroConfig, - filename: string, - source: string, - viteTransform: TransformHook, - opts: { ssr: boolean } -): Promise { +export async function cachedCompilation(props: CompileProps): Promise { + const { config, filename } = props; let cache: CompilationCache; if (!configCache.has(config)) { cache = new Map(); @@ -148,7 +153,7 @@ export async function cachedCompilation( if (cache.has(filename)) { return cache.get(filename)!; } - const compileResult = await compile(config, filename, source, viteTransform, opts); + const compileResult = await compile(props); cache.set(filename, compileResult); return compileResult; } diff --git a/packages/astro/src/vite-plugin-astro/index.ts b/packages/astro/src/vite-plugin-astro/index.ts index 9ed6f983b3..f37bbd652c 100644 --- a/packages/astro/src/vite-plugin-astro/index.ts +++ b/packages/astro/src/vite-plugin-astro/index.ts @@ -99,15 +99,21 @@ export default function astro({ config, logging }: AstroPluginOptions): vite.Plu if (isPage && config._ctx.scripts.some((s) => s.stage === 'page')) { source += `\n + +
+ + 
{ count }
+ +
+
+ +
+ + diff --git a/packages/astro/test/fixtures/component-library-shared/CounterWrapper.astro b/packages/astro/test/fixtures/component-library-shared/CounterWrapper.astro new file mode 100644 index 0000000000..35aa0773c9 --- /dev/null +++ b/packages/astro/test/fixtures/component-library-shared/CounterWrapper.astro @@ -0,0 +1,7 @@ +--- +import Counter from './Counter.jsx' +--- + + + + diff --git a/packages/astro/test/fixtures/component-library-shared/HelloReact.jsx b/packages/astro/test/fixtures/component-library-shared/HelloReact.jsx new file mode 100644 index 0000000000..4c241162d3 --- /dev/null +++ b/packages/astro/test/fixtures/component-library-shared/HelloReact.jsx @@ -0,0 +1,5 @@ +import React from 'react'; + +export default function ({ name, unused }) { + return

Hello {name}!

; +} diff --git a/packages/astro/test/fixtures/component-library-shared/README.md b/packages/astro/test/fixtures/component-library-shared/README.md new file mode 100644 index 0000000000..4489628e7b --- /dev/null +++ b/packages/astro/test/fixtures/component-library-shared/README.md @@ -0,0 +1,5 @@ +# Shared Component Library + +> This packages mimics a component library that would be published to NPM + +This used by the `component-library` test fixture to make sure that `.astro` component are resolved properly when they live outside the project's root directory. diff --git a/packages/astro/test/fixtures/component-library-shared/package.json b/packages/astro/test/fixtures/component-library-shared/package.json new file mode 100644 index 0000000000..a30f0d20f6 --- /dev/null +++ b/packages/astro/test/fixtures/component-library-shared/package.json @@ -0,0 +1,20 @@ +{ + "name": "@test/component-library-shared", + "version": "0.1.0", + "private": true, + "type": "module", + "exports": { + "./Button.astro": "./Button.astro", + "./CounterWrapper": "./CounterWrapper.astro", + "./HelloReact": "./HelloReact.jsx" + }, + "files": [ + "Button.astro", + "CounterWrapper.astro", + "Counter.svelte", + "HelloReact.jsx" + ], + "devDependencies": { + "astro": "workspace:*" + } +} diff --git a/packages/astro/test/fixtures/component-library/astro.config.mjs b/packages/astro/test/fixtures/component-library/astro.config.mjs new file mode 100644 index 0000000000..1d36918fbc --- /dev/null +++ b/packages/astro/test/fixtures/component-library/astro.config.mjs @@ -0,0 +1,8 @@ +import { defineConfig } from 'astro/config'; +import preact from '@astrojs/preact'; +import react from '@astrojs/react'; +import svelte from '@astrojs/svelte'; + +export default defineConfig({ + integrations: [preact(), react(), svelte()], +}) diff --git a/packages/astro/test/fixtures/component-library/package.json b/packages/astro/test/fixtures/component-library/package.json new file mode 100644 index 0000000000..5f13626c8a --- /dev/null +++ b/packages/astro/test/fixtures/component-library/package.json @@ -0,0 +1,14 @@ +{ + "name": "@test/component-library", + "version": "0.0.0", + "private": true, + "dependencies": { + "astro": "workspace:*", + "@astrojs/preact": "workspace:*", + "@astrojs/react": "workspace:*", + "@astrojs/svelte": "workspace:*", + "@test/component-library-shared": "workspace:*", + "react": "^18.0.0", + "react-dom": "^18.0.0" + } +} diff --git a/packages/astro/test/fixtures/component-library/src/pages/internal-hydration.astro b/packages/astro/test/fixtures/component-library/src/pages/internal-hydration.astro new file mode 100644 index 0000000000..59eb3f7424 --- /dev/null +++ b/packages/astro/test/fixtures/component-library/src/pages/internal-hydration.astro @@ -0,0 +1,17 @@ +--- +import CounterWrapper from '@test/component-library-shared/CounterWrapper'; +const title = 'With Astro Component'; +--- + + + + {title} + + +

{title}

+ + +

Hello, Svelte!

+ + + diff --git a/packages/astro/test/fixtures/component-library/src/pages/with-astro.astro b/packages/astro/test/fixtures/component-library/src/pages/with-astro.astro new file mode 100644 index 0000000000..36c5a1f5b6 --- /dev/null +++ b/packages/astro/test/fixtures/component-library/src/pages/with-astro.astro @@ -0,0 +1,15 @@ +--- +import Button from '@test/component-library-shared/Button.astro'; +const title = 'With Astro Component'; +--- + + + + {title} + + +

{title}

+ + + + diff --git a/packages/astro/test/fixtures/component-library/src/pages/with-react.astro b/packages/astro/test/fixtures/component-library/src/pages/with-react.astro new file mode 100644 index 0000000000..03cf6709c0 --- /dev/null +++ b/packages/astro/test/fixtures/component-library/src/pages/with-react.astro @@ -0,0 +1,7 @@ +--- +import HelloReact from '@test/component-library-shared/HelloReact'; +--- + + + + diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ae04af1c5e..858ea8f0a4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -951,6 +951,30 @@ importers: dependencies: astro: link:../../.. + packages/astro/test/fixtures/component-library: + specifiers: + '@astrojs/preact': workspace:* + '@astrojs/react': workspace:* + '@astrojs/svelte': workspace:* + '@test/component-library-shared': workspace:* + astro: workspace:* + react: ^18.0.0 + react-dom: ^18.0.0 + dependencies: + '@astrojs/preact': link:../../../../integrations/preact + '@astrojs/react': link:../../../../integrations/react + '@astrojs/svelte': link:../../../../integrations/svelte + '@test/component-library-shared': link:../component-library-shared + astro: link:../../.. + react: 18.0.0 + react-dom: 18.0.0_react@18.0.0 + + packages/astro/test/fixtures/component-library-shared: + specifiers: + astro: workspace:* + devDependencies: + astro: link:../../.. + packages/astro/test/fixtures/config-host: specifiers: astro: workspace:*