Repository: ProjectEvergreen/greenwood (js)

Issue:
`<script>` tags require specific ordering of attributes or else become `type="module-shim"`
## Type of Change
- [ ] New Feature Request
- [ ] Documentation / Website
- [ ] Improvement / Suggestion
- [x] Bug
- [ ] Other (please clarify below)

## Summary
Noticed as part of #554 that if you didn't organize the attributes of your `<script>` tag a specific way, your script tag would come out with the wrong type in production.

For example, this works
```html
<script type="module" src="..."></script>
```

```html
<script src="..." type="module"></script>
```

will come out as `type="module-shim"` in production builds (and so of course won't load / work)
```html
<script src="..." type="module-shim"></script>
```

## Details
This is because in [**plugin-standard-thml**](https://github.com/ProjectEvergreen/greenwood/blob/release/0.10.0/packages/cli/src/plugins/resource/plugin-standard-html.js#L272), the regex expects a pattern like this:
```js
/<script type="module-shim"/
```

Which mean it will only handle `<script>` tags of the `type` attribute comes first.  Matching it to the pattern in [**plugin-node-modules**](https://github.com/ProjectEvergreen/greenwood/blob/release/0.10.0/packages/cli/src/plugins/resource/plugin-node-modules.js#L212) would fix it.
```js
/type="module-shim"/
```
---

One thought is to scope this kind of change to only what is in the `<head>` tag, to avoid clobbering a user's content.

Pull request:
Bug/issue 555 handle script and link tag attribute ordering
<!--
## Submitting a Pull Request
We love contributions and appreciate any help you can offer!
-->

## Related Issue
resolves #555 

## Summary of Changes
1. Make `<script>` and `<link>` tag manipulation less dependent on attribute ordering
1. Limit manipulation to `<head>` tag only
1. Update test cases to mix attributes and test for content collisions

Patch:
diff --git a/packages/cli/src/plugins/resource/plugin-node-modules.js b/packages/cli/src/plugins/resource/plugin-node-modules.js
index 836a25be..2195ff52 100644
--- a/packages/cli/src/plugins/resource/plugin-node-modules.js
+++ b/packages/cli/src/plugins/resource/plugin-node-modules.js
@@ -208,8 +208,18 @@ class NodeModulesResource extends ResourceInterface {
       try {
         const { userWorkspace } = this.compilation.context;
         let newContents = body;
-        
-        newContents = newContents.replace(/type="module"/g, 'type="module-shim"');
+        const hasHead = body.match(/\<head>(.*)<\/head>/s);
+
+        if (hasHead && hasHead.length > 0) {
+          const contents = hasHead[0]
+            .replace(/type="module"/g, 'type="module-shim"');
+
+          newContents = newContents.replace(/\<head>(.*)<\/head>/s, `
+            <head>
+              ${contents}
+            </head>
+          `);
+        }
 
         const userPackageJson = fs.existsSync(`${userWorkspace}/package.json`)
           ? require(path.join(userWorkspace, 'package.json')) // its a monorepo?
diff --git a/packages/cli/src/plugins/resource/plugin-standard-html.js b/packages/cli/src/plugins/resource/plugin-standard-html.js
index 98c19100..982560d3 100644
--- a/packages/cli/src/plugins/resource/plugin-standard-html.js
+++ b/packages/cli/src/plugins/resource/plugin-standard-html.js
@@ -301,10 +301,22 @@ class StandardHtmlResource extends ResourceInterface {
   async optimize(url, body) {
     return new Promise((resolve, reject) => {
       try {
-        body = body.replace(/<script src="\/node_modules\/@webcomponents\/webcomponentsjs\/webcomponents-bundle.js"><\/script>/, '');
-        body = body.replace(/<script type="importmap-shim">.*?<\/script>/s, '');
-        body = body.replace(/<script defer="" src="\/node_modules\/es-module-shims\/dist\/es-module-shims.js"><\/script>/, '');
-        body = body.replace(/<script type="module-shim"/g, '<script type="module"');
+        const hasHead = body.match(/\<head>(.*)<\/head>/s);
+
+        if (hasHead && hasHead.length > 0) {
+          let contents = hasHead[0];
+
+          contents = contents.replace(/<script src="\/node_modules\/@webcomponents\/webcomponentsjs\/webcomponents-bundle.js"><\/script>/, '');
+          contents = contents.replace(/<script type="importmap-shim">.*?<\/script>/s, '');
+          contents = contents.replace(/<script defer="" src="\/node_modules\/es-module-shims\/dist\/es-module-shims.js"><\/script>/, '');
+          contents = contents.replace(/type="module-shim"/g, 'type="module"');
+
+          body = body.replace(/\<head>(.*)<\/head>/s, `
+            <head>
+              ${contents}
+            </head>
+          `);
+        }
     
         resolve(body);
       } catch (e) {


Test patch:
diff --git a/packages/cli/test/cases/build.default.workspace-javascript-css/build.default.workspace-javascript-css.spec.js b/packages/cli/test/cases/build.default.workspace-javascript-css/build.default.workspace-javascript-css.spec.js
index 43686ee9..631b95d3 100644
--- a/packages/cli/test/cases/build.default.workspace-javascript-css/build.default.workspace-javascript-css.spec.js
+++ b/packages/cli/test/cases/build.default.workspace-javascript-css/build.default.workspace-javascript-css.spec.js
@@ -17,9 +17,10 @@
  *     index.html
  *   scripts/
  *     main.js
+ *     other.js
  *   styles/
  *     main.css
- *     
+ *     other.css
  */
 const expect = require('chai').expect;
 const glob = require('glob-promise');
@@ -85,8 +86,8 @@ describe('Build Greenwood With: ', function() {
       });
     });
 
-    describe('<script>...</script> tag in the <head>', function() {
-      it('should have one <script> tag with inline script in the <head>', function() {
+    describe('<script>...</script> tag in the <head> with mixed attribute ordering', function() {
+      it('should have two <script> tag with inline script in the <head>', function() {
         const scriptTagInline = dom.window.document.querySelectorAll('head > script:not([src])');
         
         expect(scriptTagInline.length).to.be.equal(1);
@@ -122,17 +123,21 @@ describe('Build Greenwood With: ', function() {
       });
     });
 
-    describe('<link rel="stylesheet" href="..."/> tag in the <head>', function() {
-      it('should have one <link> tag in the <head>', function() {
+    describe('<link rel="stylesheet" href="..."/> tag in the <head> with mixed attribute ordering', function() {
+      it('should have two <link> tag in the <head>', function() {
         const linkTags = dom.window.document.querySelectorAll('head > link[rel="stylesheet"]');
         
-        expect(linkTags.length).to.be.equal(1);
+        expect(linkTags.length).to.be.equal(2);
       });
 
       it('should have the expected main.css file in the output directory', async function() {
         expect(await glob.promise(path.join(this.context.publicDir, 'styles', 'main.*.css'))).to.have.lengthOf(1);
       });
 
+      it('should have the expected other.css file in the output directory', async function() {
+        expect(await glob.promise(path.join(this.context.publicDir, 'styles', 'other.*.css'))).to.have.lengthOf(1);
+      });
+
       // JSDOM may not support this case of computing styles when using a <link> tag?
       // https://github.com/jsdom/jsdom/issues/2986
       xit('should have the color style for the output element', function() {
@@ -142,6 +147,15 @@ describe('Build Greenwood With: ', function() {
         expect(computedStyle.color).to.equal('blue');
       });
     });
+
+    describe('untouched content in the <body> for type="module-shim"', function() {
+      it('should have two <link> tag in the <head>', function() {
+        const output = dom.window.document.querySelectorAll('p.module-shim');
+
+        expect(output.length).to.be.equal(1);
+        expect(output[0].textContent).to.be.equal('let me tell you about type="module-shim"');
+      });
+    });
   });
 
   after(function() {
diff --git a/packages/cli/test/cases/build.default.workspace-javascript-css/src/pages/index.html b/packages/cli/test/cases/build.default.workspace-javascript-css/src/pages/index.html
index 8f838962..9e75fddd 100644
--- a/packages/cli/test/cases/build.default.workspace-javascript-css/src/pages/index.html
+++ b/packages/cli/test/cases/build.default.workspace-javascript-css/src/pages/index.html
@@ -3,7 +3,7 @@
 
   <head>
     <script type="module" src="../scripts/main.js"></script>
-    <script type="module" src="../scripts/other.js"></script>
+    <script src="../scripts/other.js" type="module"></script>
     
     <script type="module">
       document.getElementsByClassName('output-script-inline')[0].innerHTML = 'script tag module inline';
@@ -16,6 +16,7 @@
     </style>
 
     <link rel="stylesheet" href="../styles/main.css"></link>
+    <link href="../styles/other.css" rel="stylesheet"></link>
   </head>
 
   <body>
@@ -23,6 +24,7 @@
     <p class="output-script-inline"></p>
     <p class="output-style"></p>
     <p class="output-link"></p>
+    <p class="module-shim">let me tell you about type="module-shim"</p>
   </body>
 
 </html>
\ No newline at end of file
diff --git a/packages/cli/test/cases/build.default.workspace-javascript-css/src/styles/other.css b/packages/cli/test/cases/build.default.workspace-javascript-css/src/styles/other.css
new file mode 100644
index 00000000..f7c45488
--- /dev/null
+++ b/packages/cli/test/cases/build.default.workspace-javascript-css/src/styles/other.css
@@ -0,0 +1,3 @@
+* {
+  font-family: 'Comic Sans';
+}
\ No newline at end of file