Repository: ProjectEvergreen/greenwood (js)

Issue:
query string parameters in node_modules (CSS) paths returns no content and `204` response
## Type of Change
- [ ] New Feature Request
- [ ] Documentation / Website
- [ ] Improvement / Suggestion
- [x] Bug
- [ ] Other (please clarify below)

## Summary
As part of testing development of theme packs (#689) it was observed that when a CSS file was referenced [via an `import`](https://github.com/thescientist13/greenwood-starter-presentation/blob/master/src/components/presenter-mode.js#L2), e.g.
```js
import themeCss from '/node_modules/<package>/dist/theme.css?type=css';
```

<img width="1667" alt="Screen Shot 2021-08-07 at 5 15 27 PM" src="https://user-images.githubusercontent.com/895923/128614019-2380bc31-5087-407d-9b22-5565f096120f.png">

```sh
url /node_modules/greenwood-starter-presentation/dist/styles/theme.css?type=css

  TypeError: Cannot read property 'replace' of undefined
      at /Users/owenbuckley/Workspace/github/repos/greenwood-starter-presentation/node_modules/@greenwood/plugin-import-css/src/index.js:33:50
      at new Promise (<anonymous>)
      at ImportCssResource.intercept (/Users/owenbuckley/Workspace/github/repos/greenwood-starter-presentation/node_modules/@greenwood/plugin-import-css/src/index.js:31:12)
      at /Users/owenbuckley/Workspace/github/repos/greenwood-starter-presentation/node_modules/@greenwood/cli/src/lifecycles/serve.js:106:52
      at processTicksAndRejections (internal/process/task_queues.js:93:5)
      at async /Users/owenbuckley/Workspace/github/repos/greenwood-starter-presentation/node_modules/@greenwood/cli/src/lifecycles/serve.js:97:24
      at async /Users/owenbuckley/Workspace/github/repos/greenwood-starter-presentation/node_modules/@greenwood/cli/src/lifecycles/serve.js:97:24
      at async /Users/owenbuckley/Workspace/github/repos/greenwood-starter-presentation/node_modules/@greenwood/cli/src/lifecycles/serve.js:96:29
      at async /Users/owenbuckley/Workspace/github/repos/greenwood-starter-presentation/node_modules/@greenwood/cli/src/lifecycles/serve.js:86:5
      at async /Users/owenbuckley/Workspace/github/repos/greenwood-starter-presentation/node_modules/@greenwood/cli/src/lifecycles/serve.js:49:5
```

In this case, The URL being passed to plugin-standard-css was still including the query string and thus failing the extension check, [since `path.extname`](https://github.com/ProjectEvergreen/greenwood/blob/v0.15.0/packages/cli/src/plugins/resource/plugin-standard-css.js#L21) was also including the query string `.css?type=css` and not just `.css`, and thus failing the test;
 

## Details
This [dovetails a little more into a discussion](https://github.com/ProjectEvergreen/greenwood/discussions/691) I opened to get a better handling on handling URLs and filesystem access, so will defer to that and so will just try and land a simple patch for this for now that tries to address the _resolver_ issue detailed in that discussion

After Greenwood resolvers go first, the results of those never get passed to next plugins, because we always pass the original URL to each resolvers, instead of saying forwarding the results of the node_modules resolver to any other resolvers that may want to reuse that logic.
```js
  ctx.url = await resources.reduce(async (responsePromise, resource) => {
    const response = await responsePromise;
    // const { url } = ctx; just keep using the resolved response
    const resourceShouldResolveUrl = await resource.shouldResolve(response);  // passing response here instead
    
    return resourceShouldResolveUrl
      ? resource.resolve(response) // passing response here instead
      : Promise.resolve(response);
  }, Promise.resolve(ctx.url));
```

Thus, in the above situation, with the existing code, the query string was always getting passed to plugin-standard-css and it wasn't able to handle that since we've never tested it with _node_modules_ paths + query strings.

Pull request:
fix consistency with user plugin node modules resolution
<!--
## Submitting a Pull Request
We love contributions and appreciate any help you can offer!
-->

## Related Issue
resolves #690 

## Summary of Changes
1. Have server resolve middleware "forward" resolved URLs to each resolver (instead of the original URL each time)
1. Have plugin-node-modules resolve to paths _without_ query strings
1. Have plugin-standard-css use default `ResourceInterface.shouldResolve` logic, like most other Greenwood standard resource plugins
1. Added tests

> _Created a discussion to go into this with more depth - #691_

## TODO
1. [x] Should confirm if this is a regression or not due to #520 
    - it still existed in `v0.14.2` - https://github.com/ProjectEvergreen/greenwood/pull/692#issuecomment-894721821

Patch:
diff --git a/packages/cli/src/lifecycles/serve.js b/packages/cli/src/lifecycles/serve.js
index a845ea77..4287e66a 100644
--- a/packages/cli/src/lifecycles/serve.js
+++ b/packages/cli/src/lifecycles/serve.js
@@ -33,11 +33,10 @@ function getDevServer(compilation) {
   app.use(async (ctx, next) => {
     ctx.url = await resources.reduce(async (responsePromise, resource) => {
       const response = await responsePromise;
-      const { url } = ctx;
-      const resourceShouldResolveUrl = await resource.shouldResolve(url);
+      const resourceShouldResolveUrl = await resource.shouldResolve(response);
       
       return resourceShouldResolveUrl
-        ? resource.resolve(url)
+        ? resource.resolve(response)
         : Promise.resolve(response);
     }, Promise.resolve(ctx.url));
 
diff --git a/packages/cli/src/plugins/resource/plugin-node-modules.js b/packages/cli/src/plugins/resource/plugin-node-modules.js
index 7cf137ef..8b0b6c19 100644
--- a/packages/cli/src/plugins/resource/plugin-node-modules.js
+++ b/packages/cli/src/plugins/resource/plugin-node-modules.js
@@ -207,12 +207,13 @@ class NodeModulesResource extends ResourceInterface {
 
   async resolve(url) {
     const { projectDirectory } = this.compilation.context;
-    const isAbsoluteNodeModulesFile = fs.existsSync(path.join(projectDirectory, url));
+    const bareUrl = this.getBareUrlPath(url);
+    const isAbsoluteNodeModulesFile = fs.existsSync(path.join(projectDirectory, bareUrl));
     const nodeModulesUrl = isAbsoluteNodeModulesFile
-      ? path.join(projectDirectory, url)
-      : this.resolveRelativeUrl(projectDirectory, url)
-        ? path.join(projectDirectory, this.resolveRelativeUrl(projectDirectory, url))
-        : url;
+      ? path.join(projectDirectory, bareUrl)
+      : this.resolveRelativeUrl(projectDirectory, bareUrl)
+        ? path.join(projectDirectory, this.resolveRelativeUrl(projectDirectory, bareUrl))
+        : bareUrl;
 
     return Promise.resolve(nodeModulesUrl);
   }
diff --git a/packages/cli/src/plugins/resource/plugin-standard-css.js b/packages/cli/src/plugins/resource/plugin-standard-css.js
index 1c2ee89f..fbbf2f8f 100644
--- a/packages/cli/src/plugins/resource/plugin-standard-css.js
+++ b/packages/cli/src/plugins/resource/plugin-standard-css.js
@@ -17,12 +17,6 @@ class StandardCssResource extends ResourceInterface {
     this.contentType = 'text/css';
   }
 
-  async shouldServe(url) {
-    const isCssFile = path.extname(url) === this.extensions[0];
-    
-    return Promise.resolve(isCssFile);
-  }
-
   async serve(url) {
     return new Promise(async (resolve, reject) => {
       try {  


Test patch:
diff --git a/packages/cli/test/cases/develop.default/develop.default.spec.js b/packages/cli/test/cases/develop.default/develop.default.spec.js
index f6ade8e8..9b4d59ce 100644
--- a/packages/cli/test/cases/develop.default/develop.default.spec.js
+++ b/packages/cli/test/cases/develop.default/develop.default.spec.js
@@ -82,6 +82,14 @@ describe('Develop Greenwood With: ', function() {
         `${process.cwd()}/node_modules/lit-html/lit-html.js.map`,
         `${outputPath}/node_modules/lit-html/`
       );
+      const simpleCss = await getDependencyFiles(
+        `${process.cwd()}/node_modules/simpledotcss/simple.css`,
+        `${outputPath}/node_modules/simpledotcss/`
+      );
+      const simpleCssPackageJson = await getDependencyFiles(
+        `${process.cwd()}/node_modules/simpledotcss/package.json`,
+        `${outputPath}/node_modules/simpledotcss/`
+      );
 
       await runner.setup(outputPath, [
         ...getSetupFiles(outputPath),
@@ -91,7 +99,9 @@ describe('Develop Greenwood With: ', function() {
         ...litHtmlPackageJson,
         ...litHtml,
         ...litHtmlLibs,
-        ...litHtmlSourceMap
+        ...litHtmlSourceMap,
+        ...simpleCss,
+        ...simpleCssPackageJson
       ]);
 
       return new Promise(async (resolve) => {
@@ -444,12 +454,12 @@ describe('Develop Greenwood With: ', function() {
       });
     });
 
-    describe('Develop command specific node modules resolution behavior', function() {
+    describe('Develop command specific node modules resolution behavior for JS with query string', function() {
       let response = {};
 
       before(async function() {
         return new Promise((resolve, reject) => {
-          request.get(`${hostname}:${port}/node_modules/lit-html/lit-html.js`, (err, res, body) => {
+          request.get(`${hostname}:${port}/node_modules/lit-html/lit-html.js?type=xyz`, (err, res, body) => {
             if (err) {
               reject();
             }
@@ -478,7 +488,43 @@ describe('Develop Greenwood With: ', function() {
       });
     });
 
-    // if things work correctly, this workspace file should never resolve for the equivalent node_modules file
+    describe('Develop command specific node modules resolution behavior for CSS with query string', function() {
+      let response = {};
+
+      before(async function() {
+        return new Promise((resolve, reject) => {
+          request.get({
+            url: `http://127.0.0.1:${port}/node_modules/simpledotcss/simple.css?xyz=123`
+          }, (err, res, body) => {
+            if (err) {
+              reject();
+            }
+
+            response = res;
+            response.body = body;
+
+            resolve();
+          });
+        });
+      });
+
+      it('should return a 200', function(done) {
+        expect(response.statusCode).to.equal(200);
+        done();
+      });
+
+      it('should return the correct content type', function(done) {
+        expect(response.headers['content-type']).to.equal('text/css');
+        done();
+      });
+
+      it('should correctly return CSS from the developers local files', function(done) {
+        expect(response.body).to.contain('/* Set the global variables for everything. Change these to use your own fonts/colours. */');
+        done();
+      });
+    });
+
+    // if things work correctly, this workspace file should never resolve to the equivalent node_modules file
     // https://github.com/ProjectEvergreen/greenwood/pull/687
     describe('Develop command specific workspace resolution when matching node_modules', function() {
       let response = {};