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

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixes building of non-hoisted scripts

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<string, any> {
+  return Object.fromEntries(node.attrs.map((attr) => [attr.name, attr.value]));
+}
+
 export function findAssets(document: Document) {
   return findElements(document, isAsset);
 }

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,14 +304,17 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin {
           sourceCodeLocationInfo: true,
         });
 
-        if (facadeIdMap.has(id)) {
+        // This is the module for the page-level bundle which includes
-          const bundleId = facadeIdMap.get(id)!;
+        // hoisted scripts and hydrated components.
-          const bundlePath = '/' + bundleId;
+        const pageAssetId = facadeIdMap.get(id);
+        const bundlePath = '/' + pageAssetId;
 
         // 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')) {
+          if (getAttribute(script, 'astro-script') && typeof pageAssetId === 'string') {
             if (!pageBundleAdded) {
               pageBundleAdded = true;
               const relPath = npath.posix.relative(pathname, bundlePath);
@@ -338,8 +331,9 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin {
           }
         }
 
+        // Update external scripts. These could be hoisted or in the src folder.
         for (let script of findExternalScripts(document)) {
-            if (getAttribute(script, 'astro-script')) {
+          if (getAttribute(script, 'astro-script') && typeof pageAssetId === 'string') {
             if (!pageBundleAdded) {
               pageBundleAdded = true;
               const relPath = npath.posix.relative(pathname, bundlePath);
@@ -353,6 +347,18 @@ export function rollupPluginAstroBuildHTML(options: PluginOptions): VitePlugin {
               );
             }
             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);
                 }

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');
+}

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);
+  });
 });

packages/astro/test/fixtures/astro-scripts/src/components/NoHoistClassic.astro

@@ -0,0 +1 @@
+<script src={Astro.resolve("../scripts/no_hoist_nonmodule.js")}></script>

packages/astro/test/fixtures/astro-scripts/src/components/NoHoistModule.astro

@@ -0,0 +1 @@
+<script type="module" src={Astro.resolve("../scripts/no_hoist_module.js")}></script>

packages/astro/test/fixtures/astro-scripts/src/pages/external-no-hoist-classic.astro

@@ -0,0 +1,12 @@
+---
+import NoHoistClassic from '../components/NoHoistClassic.astro';
+---
+
+<html lang="en">
+<head>
+  <title>My Page</title>
+</head>
+<body>
+  <NoHoistClassic />
+</body>
+</html>

packages/astro/test/fixtures/astro-scripts/src/pages/external-no-hoist.astro

@@ -0,0 +1,12 @@
+---
+import NoHoistModule from '../components/NoHoistModule.astro';
+---
+
+<html lang="en">
+<head>
+  <title>My Page</title>
+</head>
+<body>
+  <NoHoistModule />
+</body>
+</html>

packages/astro/test/fixtures/astro-scripts/src/scripts/no_hoist_module.js

@@ -0,0 +1 @@
+console.log('no hoist module');

packages/astro/test/fixtures/astro-scripts/src/scripts/no_hoist_nonmodule.js

@@ -0,0 +1 @@
+console.log('no hoist nonmodule');