From aeab890971e5f425f877545c674d1cb532cee754 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Wed, 22 Jun 2022 12:02:42 -0400 Subject: [PATCH] Inline small hoisted scripts (#3658) * Inline small hoisted scripts This makes it so that small hoisted scripts get inlined into the page rather than be fetched externally. * Ensure we don't inline when there are imports * Fix ts * Update tests with new url structure * Adds a changeset --- .changeset/violet-terms-live.md | 7 ++++++ packages/astro/src/core/app/index.ts | 23 ++++++++++--------- packages/astro/src/core/app/types.ts | 8 ++++++- packages/astro/src/core/build/generate.ts | 12 +++++----- packages/astro/src/core/build/page-data.ts | 2 -- packages/astro/src/core/build/static-build.ts | 10 ++++---- packages/astro/src/core/build/types.ts | 3 +-- .../core/build/vite-plugin-hoisted-scripts.ts | 23 ++++++++++++++++++- .../astro/src/core/build/vite-plugin-ssr.ts | 2 +- packages/astro/src/core/render/ssr-element.ts | 20 ++++++++++++++++ packages/astro/test/astro-dynamic.test.js | 4 ++-- packages/astro/test/astro-scripts.test.js | 21 ++++++++++++----- .../src/components/InlineShared.astro | 3 +++ .../src/pages/inline-shared-one.astro | 14 +++++++++++ .../src/pages/inline-shared-two.astro | 14 +++++++++++ 15 files changed, 129 insertions(+), 37 deletions(-) create mode 100644 .changeset/violet-terms-live.md create mode 100644 packages/astro/test/fixtures/astro-scripts/src/components/InlineShared.astro create mode 100644 packages/astro/test/fixtures/astro-scripts/src/pages/inline-shared-one.astro create mode 100644 packages/astro/test/fixtures/astro-scripts/src/pages/inline-shared-two.astro diff --git a/.changeset/violet-terms-live.md b/.changeset/violet-terms-live.md new file mode 100644 index 000000000..96ea4167e --- /dev/null +++ b/.changeset/violet-terms-live.md @@ -0,0 +1,7 @@ +--- +'astro': patch +--- + +Inlines small hoisted scripts + +This enables a perf improvement, whereby small hoisted scripts without dependencies are inlined into the HTML, rather than loaded externally. This uses `vite.build.assetInlineLimit` to determine if the script should be inlined. diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 81cfe106a..3847a4589 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -3,6 +3,7 @@ import type { EndpointHandler, ManifestData, RouteData, + SSRElement, } from '../../@types/astro'; import type { LogOptions } from '../logger/core.js'; import type { RouteInfo, SSRManifest as Manifest } from './types'; @@ -16,6 +17,7 @@ import { RouteCache } from '../render/route-cache.js'; import { createLinkStylesheetElementSet, createModuleScriptElementWithSrcSet, + createModuleScriptElement, } from '../render/ssr-element.js'; import { matchRoute } from '../routing/match.js'; export { deserializeManifest } from './common.js'; @@ -79,18 +81,17 @@ export class App { const info = this.#routeDataToRouteInfo.get(routeData!)!; const links = createLinkStylesheetElementSet(info.links, manifest.site); - const filteredScripts = info.scripts.filter( - (script) => typeof script === 'string' || script?.stage !== 'head-inline' - ) as string[]; - const scripts = createModuleScriptElementWithSrcSet(filteredScripts, manifest.site); - - // Add all injected scripts to the page. + let scripts = new Set(); for (const script of info.scripts) { - if (typeof script !== 'string' && script.stage === 'head-inline') { - scripts.add({ - props: {}, - children: script.children, - }); + if (('stage' in script)) { + if(script.stage === 'head-inline') { + scripts.add({ + props: {}, + children: script.children, + }); + } + } else { + scripts.add(createModuleScriptElement(script, manifest.site)); } } diff --git a/packages/astro/src/core/app/types.ts b/packages/astro/src/core/app/types.ts index 7410ad42a..1bbfe3511 100644 --- a/packages/astro/src/core/app/types.ts +++ b/packages/astro/src/core/app/types.ts @@ -12,7 +12,13 @@ export interface RouteInfo { routeData: RouteData; file: string; links: string[]; - scripts: Array; + scripts: + ( + // Integration injected + { children: string; stage: string } | + // Hoisted + { type: 'inline' | 'external'; value: string; } + )[]; } export type SerializedRouteInfo = Omit & { diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts index 37abb91ad..8bac241d6 100644 --- a/packages/astro/src/core/build/generate.ts +++ b/packages/astro/src/core/build/generate.ts @@ -18,7 +18,7 @@ import { debug, info } from '../logger/core.js'; import { render } from '../render/core.js'; import { createLinkStylesheetElementSet, - createModuleScriptElementWithSrcSet, + createModuleScriptsSet, } from '../render/ssr-element.js'; import { createRequest } from '../request.js'; import { getOutputFilename, isBuildingToSSR } from '../util.js'; @@ -124,7 +124,7 @@ async function generatePage( const pageInfo = getPageDataByComponent(internals, pageData.route.component); const linkIds: string[] = Array.from(pageInfo?.css ?? []); - const hoistedId = pageInfo?.hoistedScript ?? null; + const scripts = pageInfo?.hoistedScript ?? null; const pageModule = ssrEntry.pageMap.get(pageData.component); @@ -143,7 +143,7 @@ async function generatePage( pageData, internals, linkIds, - hoistedId, + scripts, mod: pageModule, renderers, }; @@ -167,7 +167,7 @@ interface GeneratePathOptions { pageData: PageBuildData; internals: BuildInternals; linkIds: string[]; - hoistedId: string | null; + scripts: { type: 'inline' | 'external', value: string } | null; mod: ComponentInstance; renderers: SSRLoadedRenderer[]; } @@ -182,7 +182,7 @@ async function generatePath( gopts: GeneratePathOptions ) { const { astroConfig, logging, origin, routeCache } = opts; - const { mod, internals, linkIds, hoistedId, pageData, renderers } = gopts; + const { mod, internals, linkIds, scripts: hoistedScripts, pageData, renderers } = gopts; // This adds the page name to the array so it can be shown as part of stats. if (pageData.route.type === 'page') { @@ -198,7 +198,7 @@ async function generatePath( ? joinPaths(astroConfig.site?.toString() || 'http://localhost/', astroConfig.base) : astroConfig.site; const links = createLinkStylesheetElementSet(linkIds.reverse(), site); - const scripts = createModuleScriptElementWithSrcSet(hoistedId ? [hoistedId] : [], site); + const scripts = createModuleScriptsSet(hoistedScripts ? [hoistedScripts] : [], site); // Add all injected scripts to the page. for (const script of astroConfig._ctx.scripts) { diff --git a/packages/astro/src/core/build/page-data.ts b/packages/astro/src/core/build/page-data.ts index 232278346..fc8be5eab 100644 --- a/packages/astro/src/core/build/page-data.ts +++ b/packages/astro/src/core/build/page-data.ts @@ -69,7 +69,6 @@ export async function collectPagesData( moduleSpecifier: '', css: new Set(), hoistedScript: undefined, - scripts: new Set(), }; clearInterval(routeCollectionLogTimeout); @@ -131,7 +130,6 @@ export async function collectPagesData( moduleSpecifier: '', css: new Set(), hoistedScript: undefined, - scripts: new Set(), }; } diff --git a/packages/astro/src/core/build/static-build.ts b/packages/astro/src/core/build/static-build.ts index cbce7ab14..fd49f9945 100644 --- a/packages/astro/src/core/build/static-build.ts +++ b/packages/astro/src/core/build/static-build.ts @@ -117,8 +117,8 @@ async function ssrBuild(opts: StaticBuildOptions, internals: BuildInternals, inp output: { format: 'esm', entryFileNames: opts.buildConfig.serverEntry, - chunkFileNames: 'chunks/chunk.[hash].mjs', - assetFileNames: 'assets/asset.[hash][extname]', + chunkFileNames: 'chunks/[name].[hash].mjs', + assetFileNames: 'assets/[name].[hash][extname]', ...viteConfig.build?.rollupOptions?.output, }, }, @@ -202,9 +202,9 @@ async function clientBuild( input: Array.from(input), output: { format: 'esm', - entryFileNames: 'entry.[hash].js', - chunkFileNames: 'chunks/chunk.[hash].js', - assetFileNames: 'assets/asset.[hash][extname]', + entryFileNames: '[name].[hash].js', + chunkFileNames: 'chunks/[name].[hash].js', + assetFileNames: 'assets/[name].[hash][extname]', ...viteConfig.build?.rollupOptions?.output, }, preserveEntrySignatures: 'exports-only', diff --git a/packages/astro/src/core/build/types.ts b/packages/astro/src/core/build/types.ts index 0d018be7d..4342da32e 100644 --- a/packages/astro/src/core/build/types.ts +++ b/packages/astro/src/core/build/types.ts @@ -19,8 +19,7 @@ export interface PageBuildData { route: RouteData; moduleSpecifier: string; css: Set; - hoistedScript: string | undefined; - scripts: Set; + hoistedScript: { type: 'inline' | 'external', value: string } | undefined; } export type AllPagesData = Record; 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 0ac874fba..ae3525250 100644 --- a/packages/astro/src/core/build/vite-plugin-hoisted-scripts.ts +++ b/packages/astro/src/core/build/vite-plugin-hoisted-scripts.ts @@ -40,6 +40,8 @@ export function vitePluginHoistedScripts( }, async generateBundle(_options, bundle) { + let assetInlineLimit = astroConfig.vite?.build?.assetsInlineLimit || 4096; + // Find all page entry points and create a map of the entry point to the hashed hoisted script. // This is used when we render so that we can add the script to the head. for (const [id, output] of Object.entries(bundle)) { @@ -48,15 +50,34 @@ export function vitePluginHoistedScripts( output.facadeModuleId && virtualHoistedEntry(output.facadeModuleId) ) { + const canBeInlined = output.imports.length === 0 && output.dynamicImports.length === 0 && + Buffer.byteLength(output.code) <= assetInlineLimit; + let removeFromBundle = false; const facadeId = output.facadeModuleId!; const pages = internals.hoistedScriptIdToPagesMap.get(facadeId)!; for (const pathname of pages) { const vid = viteID(new URL('.' + pathname, astroConfig.root)); const pageInfo = getPageDataByViteID(internals, vid); if (pageInfo) { - pageInfo.hoistedScript = id; + if(canBeInlined) { + pageInfo.hoistedScript = { + type: 'inline', + value: output.code + }; + removeFromBundle = true; + } else { + pageInfo.hoistedScript = { + type: 'external', + value: id + }; + } } } + + // Remove the bundle if it was inlined + if(removeFromBundle) { + delete bundle[id]; + } } } }, diff --git a/packages/astro/src/core/build/vite-plugin-ssr.ts b/packages/astro/src/core/build/vite-plugin-ssr.ts index 1108c8219..05ff80916 100644 --- a/packages/astro/src/core/build/vite-plugin-ssr.ts +++ b/packages/astro/src/core/build/vite-plugin-ssr.ts @@ -129,7 +129,7 @@ function buildManifest( const routes: SerializedRouteInfo[] = []; for (const pageData of eachPageData(internals)) { - const scripts = Array.from(pageData.scripts); + const scripts: SerializedRouteInfo['scripts'] = []; if (pageData.hoistedScript) { scripts.unshift(pageData.hoistedScript); } diff --git a/packages/astro/src/core/render/ssr-element.ts b/packages/astro/src/core/render/ssr-element.ts index 628294f44..2788fdace 100644 --- a/packages/astro/src/core/render/ssr-element.ts +++ b/packages/astro/src/core/render/ssr-element.ts @@ -25,6 +25,19 @@ export function createLinkStylesheetElementSet(hrefs: string[], site?: string) { return new Set(hrefs.map((href) => createLinkStylesheetElement(href, site))); } +export function createModuleScriptElement(script: { type: 'inline' | 'external'; value: string; }, site?: string): SSRElement { + if(script.type === 'external') { + return createModuleScriptElementWithSrc(script.value, site); + } else { + return { + props: { + type: 'module', + }, + children: script.value, + }; + } +} + export function createModuleScriptElementWithSrc(src: string, site?: string): SSRElement { return { props: { @@ -41,3 +54,10 @@ export function createModuleScriptElementWithSrcSet( ): Set { return new Set(srces.map((src) => createModuleScriptElementWithSrc(src, site))); } + +export function createModuleScriptsSet( + scripts: { type: 'inline' | 'external'; value: string; }[], + site?: string +): Set { + return new Set(scripts.map(script => createModuleScriptElement(script, site))); +} diff --git a/packages/astro/test/astro-dynamic.test.js b/packages/astro/test/astro-dynamic.test.js index 5fcc4596c..6f3c32198 100644 --- a/packages/astro/test/astro-dynamic.test.js +++ b/packages/astro/test/astro-dynamic.test.js @@ -36,7 +36,7 @@ describe('Dynamic components', () => { expect($('astro-island').html()).to.equal(''); // test 2: component url const href = $('astro-island').attr('component-url'); - expect(href).to.include(`/entry`); + expect(href).to.include(`/PersistentCounter`); }); }); @@ -75,6 +75,6 @@ describe('Dynamic components subpath', () => { expect($('astro-island').html()).to.equal(''); // test 2: has component url const attr = $('astro-island').attr('component-url'); - expect(attr).to.include(`blog/entry`); + expect(attr).to.include(`blog/PersistentCounter`); }); }); diff --git a/packages/astro/test/astro-scripts.test.js b/packages/astro/test/astro-scripts.test.js index 90dba2997..79fff798e 100644 --- a/packages/astro/test/astro-scripts.test.js +++ b/packages/astro/test/astro-scripts.test.js @@ -37,15 +37,16 @@ describe('Scripts (hoisted and not)', () => { // Inline page let inline = await fixture.readFile('/inline/index.html'); let $ = cheerio.load(inline); + let $el = $('script'); // test 1: Just one entry module - expect($('script')).to.have.lengthOf(1); + expect($el).to.have.lengthOf(1); // test 2: attr removed - expect($('script').attr('data-astro')).to.equal(undefined); + expect($el.attr('data-astro')).to.equal(undefined); - const entryURL = $('script').attr('src'); - const inlineEntryJS = await fixture.readFile(entryURL); + expect($el.attr('src')).to.equal(undefined); + const inlineEntryJS = $el.text(); // test 3: the JS exists expect(inlineEntryJS).to.be.ok; @@ -57,6 +58,14 @@ describe('Scripts (hoisted and not)', () => { ); }); + it('Inline scripts that are shared by multiple pages create chunks, and aren\'t inlined into the HTML', async () => { + let html = await fixture.readFile('/inline-shared-one/index.html'); + let $ = cheerio.load(html); + + expect($('script')).to.have.lengthOf(1); + expect($('script').attr('src')).to.not.equal(undefined); + }); + it('External page builds the hoisted scripts to a single bundle', async () => { let external = await fixture.readFile('/external/index.html'); let $ = cheerio.load(external); @@ -65,8 +74,8 @@ describe('Scripts (hoisted and not)', () => { expect($('script')).to.have.lengthOf(2); let el = $('script').get(1); - let entryURL = $(el).attr('src'); - let externalEntryJS = await fixture.readFile(entryURL); + expect($(el).attr('src')).to.equal(undefined, 'This should have been inlined'); + let externalEntryJS = $(el).text(); // test 2: the JS exists expect(externalEntryJS).to.be.ok; diff --git a/packages/astro/test/fixtures/astro-scripts/src/components/InlineShared.astro b/packages/astro/test/fixtures/astro-scripts/src/components/InlineShared.astro new file mode 100644 index 000000000..2be6f7d7f --- /dev/null +++ b/packages/astro/test/fixtures/astro-scripts/src/components/InlineShared.astro @@ -0,0 +1,3 @@ + diff --git a/packages/astro/test/fixtures/astro-scripts/src/pages/inline-shared-one.astro b/packages/astro/test/fixtures/astro-scripts/src/pages/inline-shared-one.astro new file mode 100644 index 000000000..a035fa8c7 --- /dev/null +++ b/packages/astro/test/fixtures/astro-scripts/src/pages/inline-shared-one.astro @@ -0,0 +1,14 @@ +--- +import InlineShared from '../components/InlineShared.astro'; +--- + + + Testing + + + + + + diff --git a/packages/astro/test/fixtures/astro-scripts/src/pages/inline-shared-two.astro b/packages/astro/test/fixtures/astro-scripts/src/pages/inline-shared-two.astro new file mode 100644 index 000000000..97f7403fc --- /dev/null +++ b/packages/astro/test/fixtures/astro-scripts/src/pages/inline-shared-two.astro @@ -0,0 +1,14 @@ +--- +import InlineShared from '../components/InlineShared.astro'; +--- + + + Testing + + + + + +