Repository: ProjectEvergreen/greenwood (js)

Issue:
none optimization setting leads to broken resource requests in production builds
<!--
# PLEASE READ FIRST
If asking a question, looking for help, or you have an idea for Greenwood, please consider opening a discussion first or chatting with us in our Discord.
-->

## Type of Change

Bug

## Summary

There is an issue with `"none"` optimization setting where are all the `<script>` and `<link>` tags come out as their original values, and because those don't exist (we only emit the bundled / hashed URLs in the final output) it leads to a bunch of 404s in the production build

![image](https://github.com/user-attachments/assets/487efbd1-d205-4d04-a762-4efbc569ccb7)

## Details

You can see a patch implementation here, where a couple changes / behavior were observed - https://github.com/thescientist13/HP_Stuff/pull/1

1. for CSS bundling, we still need to emit the "final" bundle since we need to support plugins, or else we just ship the original source contents only, which is also not desired
1. For JavaScript / Rollup we don't emit the original source contents, also because this is needed for support plugin transformations (`noop`)
1. In the standard CSS resource plugin, we should honor the configuration setting and not do the bundling (just like in JavaScript resource plugin we conditionally apply the terser plugin or not)
1. In the standard HTML resource plugin, we need to make sure `"none"` still pulls in the right optimized contents

----

So in [the docs](https://greenwoodjs.dev/docs/reference/configuration/#optimization), we say this about the setting
> With this setting, none of your JS or CSS will be minified at all.
> The best choice if you want to handle everything yourself through custom [Resource plugins](https://greenwoodjs.dev/docs/reference/plugins-api/#resource).

Some thoughts coming from this:
- Probably obvious, but we should clarify _plugin transformations_ still apply.  So maybe we just need to phrase the docs better?
- But should we also retain the original name / path?  With the current patch, we will still emit a hashed / truncated filename.  Does this fall under the disclaimer?
- Or maybe we just remove this option entirely?
- Maybe we should call this bundledContents instead of optimizedContents in the CLI code?

We might want to make a follow up issue to better align the intent / motivation behind this feature that can address all these open questions.

Pull request:
bug/issue 1352 align none optimization option with output filename and contents
<!--
## Submitting a Pull Request
We love contributions and appreciate any help you can offer!
-->

## Related Issue

resolves #1352 

## Documentation 

N / A

## Summary of Changes

1. Have standard CSS plugin honor `none` optimization setting
1. For CSS bundling, we still need to honor the contents of any plugin process
1. For JavaScript / Rollup we don't want to emit the original source contents either
1. In the standard HTML resource plugin, make sure `"none"` still pulls in the right final optimized contents
1. Update test cases

## TODO
1. [x] fix Windows build (probably a pathing issue in the test files)
1. [x] We should probably create a ticket for a completely _unbundled_ output, that doesn't even transform filename.  Basically whatever your _src/_ contents are, that is literally what will be moved over and better clarify difference / distinction between none and unbundled - https://github.com/ProjectEvergreen/greenwood/issues/1367

Patch:
diff --git a/packages/cli/src/config/rollup.config.js b/packages/cli/src/config/rollup.config.js
index a4d07608..585f7de5 100644
--- a/packages/cli/src/config/rollup.config.js
+++ b/packages/cli/src/config/rollup.config.js
@@ -120,8 +120,7 @@ function greenwoodSyncPageResourceBundlesPlugin(compilation) {
 
           if (resourceKey === facadeModuleId) {
             const { fileName } = bundles[bundle];
-            const { rawAttributes, contents } = resource;
-            const noop = rawAttributes && rawAttributes.indexOf('data-gwd-opt="none"') >= 0 || compilation.config.optimization === 'none';
+            const { contents } = resource;
             const outputPath = new URL(`./${fileName}`, outputDir);
 
             compilation.resources.set(resource.sourcePathURL.pathname, {
@@ -130,10 +129,6 @@ function greenwoodSyncPageResourceBundlesPlugin(compilation) {
               optimizedFileContents: await fs.promises.readFile(outputPath, 'utf-8'),
               contents
             });
-
-            if (noop) {
-              await fs.promises.writeFile(outputPath, contents);
-            }
           }
         }
       }
diff --git a/packages/cli/src/lifecycles/bundle.js b/packages/cli/src/lifecycles/bundle.js
index d06de1f2..551b3f89 100644
--- a/packages/cli/src/lifecycles/bundle.js
+++ b/packages/cli/src/lifecycles/bundle.js
@@ -116,7 +116,7 @@ async function bundleStyleResources(compilation, resourcePlugins) {
   const { outputDir } = compilation.context;
 
   for (const resource of compilation.resources.values()) {
-    const { contents, optimizationAttr, src = '', type } = resource;
+    const { contents, src = '', type } = resource;
 
     if (['style', 'link'].includes(type)) {
       const resourceKey = resource.sourcePathURL.pathname;
@@ -150,75 +150,70 @@ async function bundleStyleResources(compilation, resourcePlugins) {
         });
       }
 
-      // keep the original contents, but still hash the filename based on content
-      if (compilation.config.optimization === 'none' || optimizationAttr === 'none') {
-        optimizedFileContents = contents;
-      } else {
-        const url = resource.sourcePathURL;
-        const contentType = 'text/css';
-        const headers = new Headers({ 'Content-Type': contentType, 'Accept': contentType });
-        const request = new Request(url, { headers });
-        const initResponse = new Response(contents, { headers });
-
-        let response = await resourcePlugins.reduce(async (responsePromise, plugin) => {
-          const intermediateResponse = await responsePromise;
-          const shouldServe = plugin.shouldServe && await plugin.shouldServe(url, request);
-
-          if (shouldServe) {
-            const currentResponse = await plugin.serve(url, request);
-            const mergedResponse = mergeResponse(intermediateResponse.clone(), currentResponse.clone());
-
-            if (mergedResponse.headers.get('Content-Type').indexOf(contentType) >= 0) {
-              return Promise.resolve(mergedResponse.clone());
-            }
+      const url = resource.sourcePathURL;
+      const contentType = 'text/css';
+      const headers = new Headers({ 'Content-Type': contentType, 'Accept': contentType });
+      const request = new Request(url, { headers });
+      const initResponse = new Response(contents, { headers });
+
+      let response = await resourcePlugins.reduce(async (responsePromise, plugin) => {
+        const intermediateResponse = await responsePromise;
+        const shouldServe = plugin.shouldServe && await plugin.shouldServe(url, request);
+
+        if (shouldServe) {
+          const currentResponse = await plugin.serve(url, request);
+          const mergedResponse = mergeResponse(intermediateResponse.clone(), currentResponse.clone());
+
+          if (mergedResponse.headers.get('Content-Type').indexOf(contentType) >= 0) {
+            return Promise.resolve(mergedResponse.clone());
           }
+        }
 
-          return Promise.resolve(responsePromise);
-        }, Promise.resolve(initResponse));
+        return Promise.resolve(responsePromise);
+      }, Promise.resolve(initResponse));
 
-        response = await resourcePlugins.reduce(async (responsePromise, plugin) => {
-          const intermediateResponse = await responsePromise;
-          const shouldPreIntercept = plugin.shouldPreIntercept && await plugin.shouldPreIntercept(url, request, intermediateResponse.clone());
+      response = await resourcePlugins.reduce(async (responsePromise, plugin) => {
+        const intermediateResponse = await responsePromise;
+        const shouldPreIntercept = plugin.shouldPreIntercept && await plugin.shouldPreIntercept(url, request, intermediateResponse.clone());
 
-          if (shouldPreIntercept) {
-            const currentResponse = await plugin.preIntercept(url, request, intermediateResponse.clone());
-            const mergedResponse = mergeResponse(intermediateResponse.clone(), currentResponse.clone());
+        if (shouldPreIntercept) {
+          const currentResponse = await plugin.preIntercept(url, request, intermediateResponse.clone());
+          const mergedResponse = mergeResponse(intermediateResponse.clone(), currentResponse.clone());
 
-            if (mergedResponse.headers.get('Content-Type').indexOf(contentType) >= 0) {
-              return Promise.resolve(mergedResponse.clone());
-            }
+          if (mergedResponse.headers.get('Content-Type').indexOf(contentType) >= 0) {
+            return Promise.resolve(mergedResponse.clone());
           }
+        }
 
-          return Promise.resolve(responsePromise);
-        }, Promise.resolve(response.clone()));
+        return Promise.resolve(responsePromise);
+      }, Promise.resolve(response.clone()));
 
-        response = await resourcePlugins.reduce(async (responsePromise, plugin) => {
-          const intermediateResponse = await responsePromise;
-          const shouldIntercept = plugin.shouldIntercept && await plugin.shouldIntercept(url, request, intermediateResponse.clone());
+      response = await resourcePlugins.reduce(async (responsePromise, plugin) => {
+        const intermediateResponse = await responsePromise;
+        const shouldIntercept = plugin.shouldIntercept && await plugin.shouldIntercept(url, request, intermediateResponse.clone());
 
-          if (shouldIntercept) {
-            const currentResponse = await plugin.intercept(url, request, intermediateResponse.clone());
-            const mergedResponse = mergeResponse(intermediateResponse.clone(), currentResponse.clone());
+        if (shouldIntercept) {
+          const currentResponse = await plugin.intercept(url, request, intermediateResponse.clone());
+          const mergedResponse = mergeResponse(intermediateResponse.clone(), currentResponse.clone());
 
-            if (mergedResponse.headers.get('Content-Type').indexOf(contentType) >= 0) {
-              return Promise.resolve(mergedResponse.clone());
-            }
+          if (mergedResponse.headers.get('Content-Type').indexOf(contentType) >= 0) {
+            return Promise.resolve(mergedResponse.clone());
           }
+        }
 
-          return Promise.resolve(responsePromise);
-        }, Promise.resolve(response.clone()));
+        return Promise.resolve(responsePromise);
+      }, Promise.resolve(response.clone()));
 
-        response = await resourcePlugins.reduce(async (responsePromise, plugin) => {
-          const intermediateResponse = await responsePromise;
-          const shouldOptimize = plugin.shouldOptimize && await plugin.shouldOptimize(url, intermediateResponse.clone());
+      response = await resourcePlugins.reduce(async (responsePromise, plugin) => {
+        const intermediateResponse = await responsePromise;
+        const shouldOptimize = plugin.shouldOptimize && await plugin.shouldOptimize(url, intermediateResponse.clone());
 
-          return shouldOptimize
-            ? Promise.resolve(await plugin.optimize(url, intermediateResponse.clone()))
-            : Promise.resolve(responsePromise);
-        }, Promise.resolve(response.clone()));
+        return shouldOptimize
+          ? Promise.resolve(await plugin.optimize(url, intermediateResponse.clone()))
+          : Promise.resolve(responsePromise);
+      }, Promise.resolve(response.clone()));
 
-        optimizedFileContents = await response.text();
-      }
+      optimizedFileContents = await response.text();
 
       compilation.resources.set(resourceKey, {
         ...compilation.resources.get(resourceKey),
diff --git a/packages/cli/src/plugins/resource/plugin-standard-css.js b/packages/cli/src/plugins/resource/plugin-standard-css.js
index 5c4f0005..332586de 100644
--- a/packages/cli/src/plugins/resource/plugin-standard-css.js
+++ b/packages/cli/src/plugins/resource/plugin-standard-css.js
@@ -377,7 +377,9 @@ class StandardCssResource extends ResourceInterface {
   }
 
   async intercept(url, request, response) {
-    let body = bundleCss(await response.text(), url, this.compilation);
+    let body = this.compilation.config.optimization !== 'none'
+      ? bundleCss(await response.text(), url, this.compilation)
+      : await response.text();
     let headers = {};
 
     if ((request.headers.get('Accept')?.indexOf('text/javascript') >= 0 || url.searchParams?.get('polyfill') === 'type-css') && !url.searchParams.has('type')) {
diff --git a/packages/cli/src/plugins/resource/plugin-standard-html.js b/packages/cli/src/plugins/resource/plugin-standard-html.js
index 827cc70b..1639ba97 100644
--- a/packages/cli/src/plugins/resource/plugin-standard-html.js
+++ b/packages/cli/src/plugins/resource/plugin-standard-html.js
@@ -184,7 +184,7 @@ class StandardHtmlResource extends ResourceInterface {
         if (type === 'script') {
           const tag = root.querySelectorAll('script').find(script => script.getAttribute('src') === src);
 
-          if (!optimizationAttr && optimization === 'default') {
+          if (!optimizationAttr && (optimization === 'default' || optimization === 'none')) {
             const optimizedFilePath = `${basePath}/${optimizedFileName}`;
 
             body = body.replace(src, optimizedFilePath);


Test patch:
diff --git a/packages/cli/test/cases/build.config.optimization-none/build.config-optimization-none.spec.js b/packages/cli/test/cases/build.config.optimization-none/build.config-optimization-none.spec.js
index def114b6..b129229a 100644
--- a/packages/cli/test/cases/build.config.optimization-none/build.config-optimization-none.spec.js
+++ b/packages/cli/test/cases/build.config.optimization-none/build.config-optimization-none.spec.js
@@ -77,11 +77,20 @@ describe('Build Greenwood With: ', function() {
         });
 
         it('should have the expected <script> tag in the <head>', function() {
-          const scriptTags = Array.from(dom.window.document.querySelectorAll('head script[type="module"]')).filter(tag => !tag.getAttribute('data-gwd'));
+          const src = jsFiles[0].replace(this.context.publicDir.replace(/\\/g, '/'), '');
+          const scriptTags = Array.from(dom.window.document.querySelectorAll('head script[type="module"]')).filter(tag => tag.getAttribute('src') === src);
 
           expect(scriptTags.length).to.be.equal(1);
         });
 
+        it('should have the expected preload <script> tag in the <head>', function() {
+          const src = jsFiles[0].replace(this.context.publicDir.replace(/\\/g, '/'), '');
+          const scriptPreloadTags = Array.from(dom.window.document.querySelectorAll('head link[as="script"]'));
+
+          expect(scriptPreloadTags.length).to.be.equal(1);
+          expect(scriptPreloadTags[0].getAttribute('href')).to.be.equal(src);
+        });
+
         it('should contain the expected <app-header> in the <body>', function() {
           const header = dom.window.document.querySelectorAll('body app-header');
 
@@ -100,10 +109,19 @@ describe('Build Greenwood With: ', function() {
           expect(css).to.contain('{\n  margin: 0;\n  padding: 0;\n  font-family: \'Comic Sans\', sans-serif;\n}');
         });
 
-        it('should have two <link> tag in the <head>', function() {
-          const linkTags = dom.window.document.querySelectorAll('head link');
+        it('should have the expected preload <link> tag in the <head>', function() {
+          const href = cssFiles[0].replace(this.context.publicDir.replace(/\\/g, '/'), '');
+          const linkPreloadTags = Array.from(dom.window.document.querySelectorAll('head link[as="style"]'));
+
+          expect(linkPreloadTags.length).to.be.equal(1);
+          expect(linkPreloadTags[0].getAttribute('href')).to.be.equal(href);
+        });
+
+        it('should have the expected <link> tag href  for theme.css', function() {
+          const href = cssFiles[0].replace(this.context.publicDir.replace(/\\/g, '/'), '');
+          const linkTags = Array.from(dom.window.document.querySelectorAll('head link[rel="stylesheet"]')).filter(tag => tag.getAttribute('href') === href);
 
-          expect(linkTags.length).to.be.equal(2);
+          expect(linkTags.length).to.be.equal(1);
         });
       });
     });