Split broccoli-middleware into two different middlewares #21

kratiahuja · 2017-01-03T01:12:27Z

Implementation of ember-cli RFC # 80. This PR is only the broccoli-middleware side of changes.

jfelchner · 2017-01-03T01:14:53Z

I think this is going to help with some problems I've been having with the errors page. ☺️

kratiahuja · 2017-01-03T01:18:37Z

@rwjblue should we update appveyor to not run Node 0.10 version since we dropped it in travis? I have a PR for this here.

Tests pass otherwise.

hjdivad · 2017-01-03T15:24:01Z

lib/index.js

+var watcherMiddleware = require('./watcher-middleware');
+
+module.exports = {
+  'watcherServerMiddleware': watcherServerMiddleware,


What's the motivation for retaining a combined middleware?

It was more for maintaining backward compatibility. If we need to do a point release of ember-cli that is using the combined middleware and requires a bug fix in broccoli-middleware.

Seems reasonable. Personally I would not include it in master but instead create a minor version branch if a patch version became necessary. Not sure what the maintainer preference here is though. Seems like a decision for @joliss @rwjblue @stefanpenner

Yeah I am fine any path. I don't have much experience on how it works in ember-cli.

hjdivad · 2017-01-03T16:47:24Z

lib/watcher-middleware.js

+      };
+      request.headers['x-broccoli'] = broccoliInfo;
+
+      var updatedFileName = setResponseHeaders(request, response, next, {


A couple of points here:

I'm surprised to see so much of the response header set here. eg setResponseHeaders sets things like content-length which seems to be a concern of the serveAssetMiddleware not the watcherMiddleware. In this comment the only headers set up in the watcher middleware were the x-broccoli request header for tracking the output path and the Cache-Control response header as that's a pretty reasonable header to set as a default irrespective of how the request is actually fulfilled.

Why are file-specific headers set in this middleware rather than the asset serving middleware?

It seems weird for setResponseHeaders to not be a middleware but to still accept next. I'm not suggesting that it should be a middleware: I'm worried that it's currently hiding some of the control flow. Is there some way we can split up this function?

I am not adding to set any extra headers. These headers were also set previously. I think the comment was more of an example to explain the workflow. The reason file specific headers are set in the middleware is because we want to provide these as the default implementation and let anyone override it downstream if needed rather than anyone who wants to insert itself before the asset serving middleware to almost copy over those header settings.

I thought about splitting it but it wasn't very clean since it has dependencies on stat object and other things. I could do a small refactor such that it doesn't take next.

I am not adding to set any extra headers. These headers were also set previously. I think the comment was more of an example to explain the workflow. The reason file specific headers are set in the middleware is because we want to provide these as the default implementation and let anyone override it downstream if needed rather than anyone who wants to insert itself before the asset serving middleware to almost copy over those header settings.

Ah sorry I did not make my point very clearly. I totally agree with all the headers: what I'm surprised about is that they're set in the watcher middleware and not the serve asset middleware.

I thought about splitting it but it wasn't very clean since it has dependencies on stat object and other things. I could do a small refactor such that it doesn't take next.

Seems reasonable although this is relatively minor to me in comparison to figuring out why we're doing this in this middleware rather than serve assets.

Ah sorry I did not make my point very clearly. I totally agree with all the headers: what I'm surprised about is that they're set in the watcher middleware and not the serve asset middleware.

I would disagree here since there is a usecase where this will not work as expected (if we have serve asset middleware handler those headers). The purpose of the watcher middleware was to set all the generic headers and the serve file asset middleware to actually just serve from disk. The reason being: Imagine usecase where someone wants to insert a middleware between the watcher and serve asset middleware in order to serve assets themselves. If the file headers are set by the serve asset only then the custom header inserted in between will need to duplicate the behavior of setting headers. IMO that is undesirable. The custom middleware or any downstream middleware should only overwrite the headers.

Seems reasonable although this is relatively minor to me in comparison to figuring out why we're doing this in this middleware rather than serve assets.

I went ahead and refactored the code a bit so that next is not leaked into the utility function.

The purpose of the watcher middleware was to set all the generic headers and the serve file asset middleware to actually just serve from disk

I would have imagined the purpose of the watcher middleware to just be the error handling and to block the request until the watcher has finished.

The reason being: Imagine usecase where someone wants to insert a middleware between the watcher and serve asset middleware in order to serve assets themselves. If the file headers are set by the serve asset only then the custom header inserted in between will need to duplicate the behavior of setting headers.

So a couple of things:

If someone else is serving assets presumably there's something different about them, which is why they can't just fall back to the asset serving middleware. This means that at a minimum headers like Content-Length and Last-Modified are likely to be wrong.

It seems weird that they can only sometimes rely on the default headers. The headers set in setResponseHeaders depend on being able to actually find the file on disk. Someone who wants to write a middleware between the watcher and asset serving may or may not actually be serving assets from disk or not. The reason this isn't a problem with the asset serving middleware is that serving files found on disk takes precedence over the proxy, but for someone who wanted to insert their middleware before asset serving, they would want their middleware to take precedence.

if the file headers are set by the serve asset only then the custom header inserted in between will need to duplicate the behavior of setting headers.

Maybe we could move that logic into lib/utils/serve-asset.js and make it public? This way it would be still be pretty easy to write a middleware that wanted to serve a static asset just like serve assets, but whose url -> file on disk algorithm was different (eg as with fastboot which wants / -> /index.fastboot.html instead of / -> /index.html or something similar)

Sure we can do that. I'll update this PR.

hjdivad · 2017-01-03T16:49:27Z

lib/watcher-middleware.js

+      if (updatedFileName) {
+        // set the request header file name to be the updated one (could be a null if asset is not found)
+        broccoliInfo.filename = updatedFileName;
+        request.headers['x-broccoli'] = broccoliInfo;


In what ways can the file name be updated? This code looks general and the comment makes it sound general, but from what I could tell of setResponseHeaders the only change is that index.html may be appended. Did I miss something?

It can be updated when index.html is appended. Nope you didn't miss anything.

Okay cool, do we need this in the watcher middleware? This is related to my point above, where it feels to me like this stuff is the concern of the asset serving middleware rather than the watcher middleware.

Please see my response above.

hjdivad · 2017-01-03T16:54:35Z

lib/serve-asset-middleware.js

+      });
+    } else {
+      // bypass this middleware if the filename is not defined
+      next();


What's the use case for nooping here?

In ember-cli the last middleware in the chain is the proxy-server-middleware which is invoked when a file is not found in disk or if index.html and autoIndex is not true. Basically the case when someone uses ember serve --proxy http://localhost:3000 for example. In this case, when we know the file is not found we want to bypass this middleware (ie not even serve it) and let the proxy server handle the request.

kratiahuja · 2017-01-09T04:10:07Z

@hjdivad Updated per your suggestion. Please let me know if I misunderstood anything.

hjdivad

I have a number of very minor nits but nothing substantial. I think it would be fine to fix them before or after merge. High level everything looks great.

Thanks @kratiahuja 👍

hjdivad · 2017-01-11T18:08:28Z

lib/serve-asset-middleware.js

+var serveAsset = require('./utils/serve-asset');
+var setResponseHeaders = require('./utils/response-header').setResponseHeaders;
+
+module.exports = function serveAssetsMiddleware() {


I don't think serveAssetsMiddleware needs to be a function that returns a middleware. We can just export the middleware directly.

The reason the previous implementation had a function that returned a middleware was to close over the watcher argument, but that's not necessary here.

ie we can just module.exports = function(request, response, next) { // ...

I was just following the format of other middlewares for consistency. Fixed to just not have the closure 😄

hjdivad · 2017-01-11T18:13:11Z

lib/utils/response-header.js

+  var broccoliInfo = req.headers['x-broccoli'];
+  if (broccoliInfo) {
+    broccoliInfo.filename = filename;
+    req.headers['x-broccoli'] = broccoliInfo;


This line seems to be a no-op; if the above conditional matches then req.headers['x-broccoli'] == broccoliInfo already

Sorry about that. Fixed.

hjdivad · 2017-01-11T18:16:27Z

lib/watcher-middleware.js

+var url = require('url');
+var path = require('path');
+
+//var setResponseHeaders = require('./utils/response-header');


hjdivad · 2017-01-11T18:17:31Z

test/middleware-test.js

@@ -111,11 +110,10 @@ describe('broccoli-middleware', function() {
        });
    });

-    it('responds with index.html if request is a directory and autoIndex is set to false', function() {
+    it('responds with index.html if request is a directory and autoIndex is set to true', function() {


👍

Always good to make tests tell the truth 😄

hjdivad · 2017-01-11T19:01:14Z

test/serve-asset-middleware-test.js

+
+
+      server = new TestHTTPServer(middleware);
+      server.addMiddleware(wrapperMiddleware);


What does the wrapper achieve? Can't we just server.addMiddleware(serveAssetMiddleFn)

The wrapperMiddleware creates a closure to allow doing assertions. I could otherwise just create a another middleware to do the assertions after serveAssetMiddleware is finished executing.

hjdivad · 2017-01-11T19:01:37Z

test/serve-asset-middleware-test.js

+
+      server = new TestHTTPServer(middleware);
+      server.addMiddleware(wrapperMiddleware);
+


as above, unclear on what we get out of the wrapper here

The wrapperMiddleware creates a closure to allow doing assertions. I could otherwise just create a another middleware to do the assertions after serveAssetMiddleware is finished executing.

hjdivad · 2017-01-11T19:01:55Z

test/serve-asset-middleware-test.js

+      var serveMiddleware = serveAssetMiddleware();
+
+      server = new TestHTTPServer(middleware);
+      server.addMiddleware(serveMiddleware);


as above, unsure about the wrapper

The wrapperMiddleware creates a closure to allow doing assertions. I could otherwise just create a another middleware to do the assertions after serveAssetMiddleware is finished executing.

hjdivad · 2017-01-11T19:03:24Z

test/watcher-middleware-test.js

+        })
+      };
+
+      server = new TestHTTPServer(wrapperMiddleware);


what's the point of the wrapper here? can't we use middleware directly?

The wrapperMiddleware creates a closure to allow doing assertions. I could otherwise just create a another middleware to do the assertions after serveAssetMiddleware is finished executing.

Implementation of ember-cli RFC # 80.

kratiahuja · 2017-01-15T03:52:22Z

@hjdivad Thanks for your feedback. I apologize for the delay in updating the PR. I believe I have addressed the nitpicks too with the latest push. Could you please take another quick look? 😄

If this is good, could we please merge it in (only master)?

hjdivad · 2017-01-17T21:12:28Z

Looks great, thanks @kratiahuja!

kratiahuja · 2017-01-17T23:23:25Z

@hjdivad @stefanpenner @rwjblue need another favor. Can we do a release of broccoli-middleware from master as 1.0.0-beta8? I need this to get tests passing in ember-cli.

stefanpenner · 2017-01-17T23:44:28Z

releasing now

stefanpenner · 2017-01-17T23:46:04Z

should be released, let me know if there is an issue.

kratiahuja · 2017-01-18T01:28:13Z

Thank you @stefanpenner ! I'll let you know if I hit any issues.

Split serving assets into two different in-repo addons Implementation of [RFC # 80 of ember-cli](ember-cli/rfcs#80). This PR primarily does the following: - [x] Introduces `ember-cli:broccoli:watcher` addon that is responsible for setting the response headers for every asset request being served. - [x] Introduces `ember-cli:broccoli:serve-files` addon that is responsible for serving the assets - [x] Removes `serve-files` addon since the above two addon do the same work - [x] Release and update `broccoli-middleware` version after [PR](ember-cli/broccoli-middleware#21) is merged. - [x] Fix broken tests here after the `broccoli-middleware` version is updated. - [x] Update `ember-cli-inject-livereload` from `serve-files` to `ember-cli:broccoli:serve-files` [here](ember-cli/ember-cli-inject-live-reload#39) **Note**: `broccoli-middleware` output format is now changed but to maintain backward compatibility it still has the original one middleware that does both setting response headers and serving assets. TODO: - [ ] Release a major version of `ember-cli-inject-live-reload` that is compatible with the first release of ember-cli containing this PR. - [ ] Update change log?

This was referenced Jan 3, 2017

Split serving assets into two different in-repo addons ember-cli/ember-cli#6633

Merged

Expose a public API to allow assets to be served in a custom way using ember serve ember-cli/rfcs#80

Merged

hjdivad reviewed Jan 3, 2017

View reviewed changes

kratiahuja force-pushed the add-serve-api branch 4 times, most recently from 2d73225 to 6e055a5 Compare January 9, 2017 04:09

hjdivad approved these changes Jan 11, 2017

View reviewed changes

Split broccoli-middleware into two different middlewares

3e12781

Implementation of ember-cli RFC # 80.

kratiahuja force-pushed the add-serve-api branch from 6e055a5 to 3e12781 Compare January 15, 2017 03:50

hjdivad merged commit 293c6c1 into master Jan 17, 2017

hjdivad deleted the add-serve-api branch January 17, 2017 21:12



		server = new TestHTTPServer(middleware);
		server.addMiddleware(wrapperMiddleware);

Split broccoli-middleware into two different middlewares #21

Split broccoli-middleware into two different middlewares #21

Conversation

kratiahuja commented Jan 3, 2017

jfelchner commented Jan 3, 2017

kratiahuja commented Jan 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kratiahuja Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kratiahuja commented Jan 9, 2017

hjdivad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kratiahuja commented Jan 15, 2017

hjdivad commented Jan 17, 2017

kratiahuja commented Jan 17, 2017

stefanpenner commented Jan 17, 2017

stefanpenner commented Jan 17, 2017

kratiahuja commented Jan 18, 2017

kratiahuja commented Jan 3, 2017 •

edited

Loading

kratiahuja Jan 4, 2017 •

edited

Loading