From 8cb779594e585ac9241a6c0c7cc311c850cc5691 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Mon, 22 Nov 2021 16:17:01 -0500 Subject: [PATCH] Bring back building of non-hoisted scripts (#1977) * Bring back building of non-hoisted scripts Scripts inside of src/, whether hoisted are not should be built. This makes that so. If not hoisted they do *not* get bundled together, but rather are their own standalone modules. This matches 0.20 behavior. Closes #1968 * Adds a changeset * Fix windows breakage * Debug windows * More debugging * make it not be parallel * More windows * Might fix it * ARG * Simpler test * Remove the debugging --- .changeset/tiny-buckets-mix.md | 5 + .../vite-plugin-build-html/extract-assets.ts | 6 +- .../astro/src/vite-plugin-build-html/index.ts | 110 +++++++++--------- .../astro/src/vite-plugin-build-html/util.ts | 50 ++++++++ packages/astro/test/astro-scripts.test.js | 28 ++++- .../src/components/NoHoistClassic.astro | 1 + .../src/components/NoHoistModule.astro | 1 + .../src/pages/external-no-hoist-classic.astro | 12 ++ .../src/pages/external-no-hoist.astro | 12 ++ .../src/scripts/no_hoist_module.js | 1 + .../src/scripts/no_hoist_nonmodule.js | 1 + 11 files changed, 172 insertions(+), 55 deletions(-) create mode 100644 .changeset/tiny-buckets-mix.md create mode 100644 packages/astro/src/vite-plugin-build-html/util.ts create mode 100644 packages/astro/test/fixtures/astro-scripts/src/components/NoHoistClassic.astro create mode 100644 packages/astro/test/fixtures/astro-scripts/src/components/NoHoistModule.astro create mode 100644 packages/astro/test/fixtures/astro-scripts/src/pages/external-no-hoist-classic.astro create mode 100644 packages/astro/test/fixtures/astro-scripts/src/pages/external-no-hoist.astro create mode 100644 packages/astro/test/fixtures/astro-scripts/src/scripts/no_hoist_module.js create mode 100644 packages/astro/test/fixtures/astro-scripts/src/scripts/no_hoist_nonmodule.js diff --git a/.changeset/tiny-buckets-mix.md b/.changeset/tiny-buckets-mix.md new file mode 100644 index 000000000..8c40341ee --- /dev/null +++ b/.changeset/tiny-buckets-mix.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes building of non-hoisted scripts diff --git a/packages/astro/src/vite-plugin-build-html/extract-assets.ts b/packages/astro/src/vite-plugin-build-html/extract-assets.ts index b0e808ab3..c323f6efb 100644 --- a/packages/astro/src/vite-plugin-build-html/extract-assets.ts +++ b/packages/astro/src/vite-plugin-build-html/extract-assets.ts @@ -82,7 +82,7 @@ function isInlineScript(node: Element): boolean { function isExternalScript(node: Element): boolean { switch (getTagName(node)) { case 'script': - if (getAttribute(node, 'type') === 'module' && hasAttribute(node, 'src')) { + if (hasAttribute(node, 'src')) { return true; } return false; @@ -191,6 +191,10 @@ export function getTextContent(node: Node): string { return subtree.map(getTextContent).join(''); } +export function getAttributes(node: Element): Record { + return Object.fromEntries(node.attrs.map((attr) => [attr.name, attr.value])); +} + export function findAssets(document: Document) { return findElements(document, isAsset); } diff --git a/packages/astro/src/vite-plugin-build-html/index.ts b/packages/astro/src/vite-plugin-build-html/index.ts index bdcf7b5ff..9a3254c93 100644 --- a/packages/astro/src/vite-plugin-build-html/index.ts +++ b/packages/astro/src/vite-plugin-build-html/index.ts @@ -7,9 +7,10 @@ import parse5 from 'parse5'; import srcsetParse from 'srcset-parse'; import * as npath from 'path'; import { promises as fs } from 'fs'; -import { getAttribute, hasAttribute, getTagName, insertBefore, remove, createScript, createElement, setAttribute } from '@web/parse5-utils'; +import { getAttribute, hasAttribute, insertBefore, remove, createScript, createElement, setAttribute } from '@web/parse5-utils'; import { addRollupInput } from './add-rollup-input.js'; -import { findAssets, findExternalScripts, findInlineScripts, findInlineStyles, getTextContent, isStylesheetLink } from './extract-assets.js'; +import { findAssets, findExternalScripts, findInlineScripts, findInlineStyles, getTextContent, getAttributes } from './extract-assets.js'; +import { isBuildableImage, isBuildableLink, isHoistedScript, isInSrcDirectory, hasSrcSet } from './util.js'; import { render as ssrRender } from '../core/ssr/index.js'; import { getAstroStyleId, getAstroPageStyleId } from '../vite-plugin-build-css/index.js'; @@ -22,19 +23,6 @@ const ASTRO_SCRIPT_PREFIX = '@astro-script'; const ASTRO_EMPTY = '@astro-empty'; const STATUS_CODE_RE = /^404$/; -const tagsWithSrcSet = new Set(['img', 'source']); - -const isAstroInjectedLink = (node: parse5.Element) => isStylesheetLink(node) && getAttribute(node, 'data-astro-injected') === ''; -const isBuildableLink = (node: parse5.Element, srcRoot: string, srcRootWeb: string) => { - if (isAstroInjectedLink(node)) return true; - const href = getAttribute(node, 'href'); - if (typeof href !== 'string' || !href.length) return false; - return href.startsWith(srcRoot) || href.startsWith(srcRootWeb) || `/${href}`.startsWith(srcRoot); // Windows fix: some paths are missing leading "/" -}; -const isBuildableImage = (node: parse5.Element, srcRoot: string, srcRootWeb: string) => - getTagName(node) === 'img' && (getAttribute(node, 'src')?.startsWith(srcRoot) || getAttribute(node, 'src')?.startsWith(srcRootWeb)); -const hasSrcSet = (node: parse5.Element) => tagsWithSrcSet.has(getTagName(node)) && !!getAttribute(node, 'srcset'); -const isHoistedScript = (node: parse5.Element) => getTagName(node) === 'script' && hasAttribute(node, 'hoist'); interface PluginOptions { astroConfig: AstroConfig; @@ -118,7 +106,6 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin { for (const script of findExternalScripts(document)) { if (isHoistedScript(script)) { - debugger; const astroScript = getAttribute(script, 'astro-script'); const src = getAttribute(script, 'src'); if (astroScript) { @@ -127,6 +114,9 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin { frontEndImports.push(scriptId); astroScriptMap.set(scriptId, js); } + } else if(isInSrcDirectory(script, 'src', srcRoot, srcRootWeb)) { + const src = getAttribute(script, 'src'); + if(src) jsInput.add(src); } } @@ -314,44 +304,60 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin { sourceCodeLocationInfo: true, }); - if (facadeIdMap.has(id)) { - const bundleId = facadeIdMap.get(id)!; - const bundlePath = '/' + bundleId; + // This is the module for the page-level bundle which includes + // hoisted scripts and hydrated components. + const pageAssetId = facadeIdMap.get(id); + const bundlePath = '/' + pageAssetId; - // Update scripts - let pageBundleAdded = false; - for (let script of findInlineScripts(document)) { - if (getAttribute(script, 'astro-script')) { - if (!pageBundleAdded) { - pageBundleAdded = true; - const relPath = npath.posix.relative(pathname, bundlePath); - insertBefore( - script.parentNode, - createScript({ - type: 'module', - src: relPath, - }), - script - ); - } - remove(script); + // Update scripts + let pageBundleAdded = false; + + // Update inline scripts. These could be hydrated component scripts or hoisted inline scripts + for (let script of findInlineScripts(document)) { + if (getAttribute(script, 'astro-script') && typeof pageAssetId === 'string') { + if (!pageBundleAdded) { + pageBundleAdded = true; + const relPath = npath.posix.relative(pathname, bundlePath); + insertBefore( + script.parentNode, + createScript({ + type: 'module', + src: relPath, + }), + script + ); } + remove(script); } + } - for (let script of findExternalScripts(document)) { - if (getAttribute(script, 'astro-script')) { - if (!pageBundleAdded) { - pageBundleAdded = true; - const relPath = npath.posix.relative(pathname, bundlePath); - insertBefore( - script.parentNode, - createScript({ - type: 'module', - src: relPath, - }), - script - ); - } + // Update external scripts. These could be hoisted or in the src folder. + for (let script of findExternalScripts(document)) { + if (getAttribute(script, 'astro-script') && typeof pageAssetId === 'string') { + if (!pageBundleAdded) { + pageBundleAdded = true; + const relPath = npath.posix.relative(pathname, bundlePath); + insertBefore( + script.parentNode, + createScript({ + type: 'module', + src: relPath, + }), + script + ); + } + remove(script); + } else if(isInSrcDirectory(script, 'src', srcRoot, srcRootWeb)) { + const src = getAttribute(script, 'src'); + // On windows the facadeId doesn't start with / but does not Unix :/ + if(src && (facadeIdMap.has(src) || facadeIdMap.has(src.substr(1)))) { + const assetRootPath = '/' + (facadeIdMap.get(src) || facadeIdMap.get(src.substr(1))); + const relPath = npath.posix.relative(pathname, assetRootPath); + const attrs = getAttributes(script); + insertBefore(script.parentNode, createScript({ + ...attrs, + src: relPath + }), script); remove(script); } } @@ -365,7 +371,7 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin { switch (rel) { case 'stylesheet': { if (!pageCSSAdded) { - const attrs = Object.fromEntries(node.attrs.map((attr) => [attr.name, attr.value])); + const attrs = getAttributes(node); delete attrs['data-astro-injected']; pageCSSAdded = appendStyleChunksBefore(node, pathname, cssChunkMap.get(styleId), attrs); } @@ -374,7 +380,7 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin { } case 'preload': { if (getAttribute(node, 'as') === 'style') { - const attrs = Object.fromEntries(node.attrs.map((attr) => [attr.name, attr.value])); + const attrs = getAttributes(node); appendStyleChunksBefore(node, pathname, cssChunkMap.get(styleId), attrs); remove(node); } diff --git a/packages/astro/src/vite-plugin-build-html/util.ts b/packages/astro/src/vite-plugin-build-html/util.ts new file mode 100644 index 000000000..585ba771f --- /dev/null +++ b/packages/astro/src/vite-plugin-build-html/util.ts @@ -0,0 +1,50 @@ + +import { getAttribute, hasAttribute, getTagName } from '@web/parse5-utils'; +import parse5 from 'parse5'; +import { isStylesheetLink } from './extract-assets.js'; + +const tagsWithSrcSet = new Set(['img', 'source']); + +function startsWithSrcRoot(pathname: string, srcRoot: string, srcRootWeb: string): boolean { + return pathname.startsWith(srcRoot) // /Users/user/project/src/styles/main.css + || pathname.startsWith(srcRootWeb) // /src/styles/main.css + || `/${pathname}`.startsWith(srcRoot); // Windows fix: some paths are missing leading "/" +} + +export function isInSrcDirectory(node: parse5.Element, attr: string, srcRoot: string, srcRootWeb: string): boolean { + const value = getAttribute(node, attr); + return value ? startsWithSrcRoot(value, srcRoot, srcRootWeb) : false; +}; + +export function isAstroInjectedLink(node: parse5.Element): boolean { + return isStylesheetLink(node) && getAttribute(node, 'data-astro-injected') === ''; +} + +export function isBuildableLink(node: parse5.Element, srcRoot: string, srcRootWeb: string): boolean { + if (isAstroInjectedLink(node)) { + return true; + } + + const href = getAttribute(node, 'href'); + if (typeof href !== 'string' || !href.length) { + return false; + } + + return startsWithSrcRoot(href, srcRoot, srcRootWeb); +}; + +export function isBuildableImage(node: parse5.Element, srcRoot: string, srcRootWeb: string): boolean { + if(getTagName(node) === 'img') { + const src = getAttribute(node, 'src'); + return src ? startsWithSrcRoot(src, srcRoot, srcRootWeb) : false; + } + return false; +} + +export function hasSrcSet(node: parse5.Element): boolean { + return tagsWithSrcSet.has(getTagName(node)) && !!getAttribute(node, 'srcset'); +} + +export function isHoistedScript(node: parse5.Element): boolean { + return getTagName(node) === 'script' && hasAttribute(node, 'hoist'); +} \ No newline at end of file diff --git a/packages/astro/test/astro-scripts.test.js b/packages/astro/test/astro-scripts.test.js index cb7e7fe1d..5d3e6cd96 100644 --- a/packages/astro/test/astro-scripts.test.js +++ b/packages/astro/test/astro-scripts.test.js @@ -3,7 +3,7 @@ import cheerio from 'cheerio'; import path from 'path'; import { loadFixture } from './test-utils.js'; -describe('Hoisted scripts', () => { +describe('Scripts (hoisted and not)', () => { let fixture; before(async () => { @@ -45,7 +45,7 @@ describe('Hoisted scripts', () => { expect(inlineEntryJS).to.be.ok; }); - it('External page builds the scripts to a single bundle', async () => { + it('External page builds the hoisted scripts to a single bundle', async () => { let external = await fixture.readFile('/external/index.html'); let $ = cheerio.load(external); @@ -59,4 +59,28 @@ describe('Hoisted scripts', () => { // test 2: the JS exists expect(externalEntryJS).to.be.ok; }); + + it('External page using non-hoist scripts that are modules are built standalone', async () => { + let external = await fixture.readFile('/external-no-hoist/index.html'); + let $ = cheerio.load(external); + + // test 1: there is 1 scripts + expect($('script')).to.have.lengthOf(1); + + // test 2: inside assets + let entryURL = $('script').attr('src'); + expect(entryURL.includes('assets/')).to.equal(true); + }); + + it('External page using non-hoist scripts that are not modules are built standalone', async () => { + let external = await fixture.readFile('/external-no-hoist-classic/index.html'); + let $ = cheerio.load(external); + + // test 1: there is 1 scripts + expect($('script')).to.have.lengthOf(1); + + // test 2: inside assets + let entryURL = $('script').attr('src'); + expect(entryURL.includes('assets/')).to.equal(true); + }); }); diff --git a/packages/astro/test/fixtures/astro-scripts/src/components/NoHoistClassic.astro b/packages/astro/test/fixtures/astro-scripts/src/components/NoHoistClassic.astro new file mode 100644 index 000000000..33addc403 --- /dev/null +++ b/packages/astro/test/fixtures/astro-scripts/src/components/NoHoistClassic.astro @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/packages/astro/test/fixtures/astro-scripts/src/components/NoHoistModule.astro b/packages/astro/test/fixtures/astro-scripts/src/components/NoHoistModule.astro new file mode 100644 index 000000000..ba20d7371 --- /dev/null +++ b/packages/astro/test/fixtures/astro-scripts/src/components/NoHoistModule.astro @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/packages/astro/test/fixtures/astro-scripts/src/pages/external-no-hoist-classic.astro b/packages/astro/test/fixtures/astro-scripts/src/pages/external-no-hoist-classic.astro new file mode 100644 index 000000000..360a58d8d --- /dev/null +++ b/packages/astro/test/fixtures/astro-scripts/src/pages/external-no-hoist-classic.astro @@ -0,0 +1,12 @@ +--- +import NoHoistClassic from '../components/NoHoistClassic.astro'; +--- + + + + My Page + + + + + \ No newline at end of file diff --git a/packages/astro/test/fixtures/astro-scripts/src/pages/external-no-hoist.astro b/packages/astro/test/fixtures/astro-scripts/src/pages/external-no-hoist.astro new file mode 100644 index 000000000..da1d2e9a2 --- /dev/null +++ b/packages/astro/test/fixtures/astro-scripts/src/pages/external-no-hoist.astro @@ -0,0 +1,12 @@ +--- +import NoHoistModule from '../components/NoHoistModule.astro'; +--- + + + + My Page + + + + + \ No newline at end of file diff --git a/packages/astro/test/fixtures/astro-scripts/src/scripts/no_hoist_module.js b/packages/astro/test/fixtures/astro-scripts/src/scripts/no_hoist_module.js new file mode 100644 index 000000000..2bacb105a --- /dev/null +++ b/packages/astro/test/fixtures/astro-scripts/src/scripts/no_hoist_module.js @@ -0,0 +1 @@ +console.log('no hoist module'); \ No newline at end of file diff --git a/packages/astro/test/fixtures/astro-scripts/src/scripts/no_hoist_nonmodule.js b/packages/astro/test/fixtures/astro-scripts/src/scripts/no_hoist_nonmodule.js new file mode 100644 index 000000000..ffa1310e6 --- /dev/null +++ b/packages/astro/test/fixtures/astro-scripts/src/scripts/no_hoist_nonmodule.js @@ -0,0 +1 @@ +console.log('no hoist nonmodule'); \ No newline at end of file