Enhancements to `options.middleware` #85

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@steve-jansen
Contributor

steve-jansen commented Feb 11, 2014

Hi,

I'm the author of a developer utility called json-proxy and love grunt! I noticed recently my documentation for injecting json-proxy as middleware into grunt-contrib-connect no longer worked properly with Yeoman.io templates.

I realized that users are copying the default middlewares function from connect.js#L31-45.

This seems really fragile, and awkward for users. See drewzboto/grunt-connect-proxy#29 as an example.

So, I enhanced the options.middleware logic to maintain backwards compatibility yet make life easier for extending the default middlewares:

  • modified options.middleware to accept an array or a function
  • modified the arguments sent to the options.middleware function to include a third argument that is the array of default middlewares

I included passing nodeunit tests and updated the relevant docs. I also refactored the tests to include coverage for HTTPS use cases (which I believe other PRs may address as well; figured it was worth fixing while I was in the codebase). Finally, I reconfigured the test cases to use a sequential set of ports starting at TCP port 8000.

Enjoy!
Steve

@vladikoff

This comment has been minimized.

Show comment
Hide comment
@vladikoff

vladikoff Feb 19, 2014

Member

Hello @steve-jansen, thanks for the PR. Could you rebase your PR and don't bump the version for us.

Member

vladikoff commented Feb 19, 2014

Hello @steve-jansen, thanks for the PR. Could you rebase your PR and don't bump the version for us.

@steve-jansen

This comment has been minimized.

Show comment
Hide comment
@steve-jansen

steve-jansen Feb 19, 2014

Contributor

Hi @vladikoff, no problem. Just pushed a commit to revert the version bump in CHANGELOG and package.json. For some reason GitHub picked up the new commit but Travis did not (yet). Let me know if I did something wrong with Travis.

Contributor

steve-jansen commented Feb 19, 2014

Hi @vladikoff, no problem. Just pushed a commit to revert the version bump in CHANGELOG and package.json. For some reason GitHub picked up the new commit but Travis did not (yet). Let me know if I did something wrong with Travis.

@vladikoff

This comment has been minimized.

Show comment
Hide comment
@vladikoff

vladikoff Feb 19, 2014

Member

Could you squash your commits into one? Thanks!

Member

vladikoff commented Feb 19, 2014

Could you squash your commits into one? Thanks!

@steve-jansen

This comment has been minimized.

Show comment
Hide comment
@steve-jansen

steve-jansen Feb 19, 2014

Contributor

Hi @vladikoff, just pushed a squashed commit. I prefer squashed commits too, but, wasn't sure if that was kosher since I had pushed my commits to origin. Thinking about it now, it makes sense that squashing and force pushing to my fork wouldn't harm the mainline repo. Learn something new everyday!

Contributor

steve-jansen commented Feb 19, 2014

Hi @vladikoff, just pushed a squashed commit. I prefer squashed commits too, but, wasn't sure if that was kosher since I had pushed my commits to origin. Thinking about it now, it makes sense that squashing and force pushing to my fork wouldn't harm the mainline repo. Learn something new everyday!

@vladikoff

This comment has been minimized.

Show comment
Hide comment
@vladikoff

vladikoff Feb 19, 2014

Member

Oh seems like you still need to git rebase from current master.

Member

vladikoff commented Feb 19, 2014

Oh seems like you still need to git rebase from current master.

enhance options.middleware to pass the default middleware as an arg
- modified options.middleware to accept an array or a function

- modified the arguments sent to the options.middleware function to
  include a third argument that is the array of default middlewares

- added unit tests for https use cases and new options.middleware
  use cases

- refactored unit tests and the test case configs in Gruntfile.js to
  use a sequential set of TCP port numbers, starting at 8000,
  for clarity
@steve-jansen

This comment has been minimized.

Show comment
Hide comment
@steve-jansen

steve-jansen Feb 19, 2014

Contributor

Hi @vladikoff, you have been busy today! Just rebased from the upstream master and pushed again. Travis is now building successfully; guess Travis needs automatic merging to kick off a build against a pull request.

Contributor

steve-jansen commented Feb 19, 2014

Hi @vladikoff, you have been busy today! Just rebased from the upstream master and pushed again. Travis is now building successfully; guess Travis needs automatic merging to kick off a build against a pull request.

@@ -35,18 +35,21 @@ module.exports = function(grunt) {
},
custom_port: {
options: {
- port: 9000,
+ base: 'test',
+ port: 8001,

This comment has been minimized.

@vladikoff

vladikoff Feb 19, 2014

Member

Why did you have to update all of these?

@vladikoff

vladikoff Feb 19, 2014

Member

Why did you have to update all of these?

This comment has been minimized.

@steve-jansen

steve-jansen Feb 20, 2014

Contributor

Starting at 8000 was purely an arbitrary decision to sequentially number the ports to make it obvious when a server was running on a port yet no tests were being made against that server instance.

If you look at the 0fc962 test/connect_test.js#L24-83 and 0fc962 Gruntfile.js#L37-90, you will see 7 servers but only 4 test suites, with a mix of ports in the 8000 and 9000 ranges.

@steve-jansen

steve-jansen Feb 20, 2014

Contributor

Starting at 8000 was purely an arbitrary decision to sequentially number the ports to make it obvious when a server was running on a port yet no tests were being made against that server instance.

If you look at the 0fc962 test/connect_test.js#L24-83 and 0fc962 Gruntfile.js#L37-90, you will see 7 servers but only 4 test suites, with a mix of ports in the 8000 and 9000 ranges.

- middlewares.push(connect.directory(directory));
- return middlewares;
- }
+ middleware: null

This comment has been minimized.

@vladikoff

vladikoff Feb 19, 2014

Member

Confused here, this is now null, but the docs have

...
function myMiddleware(req, res, next) {
            res.end('Hello, world!'');
          }

as the Default value.

@vladikoff

vladikoff Feb 19, 2014

Member

Confused here, this is now null, but the docs have

...
function myMiddleware(req, res, next) {
            res.end('Hello, world!'');
          }

as the Default value.

This comment has been minimized.

@steve-jansen

steve-jansen Feb 20, 2014

Contributor

The null middleware will be initialized to a default array of middlewares on line 88 of the same file.

middleware = createDefaultMiddleware.call(this, connect, options);

My intent in the docs is to document that the default array of middlewares will be used when the value of options.middleware is null or undefined. The subsequent lines in the documentation that you reference provide simple examples of passing a non-null value, either as an Array or as a function.

Does this work for you?

@steve-jansen

steve-jansen Feb 20, 2014

Contributor

The null middleware will be initialized to a default array of middlewares on line 88 of the same file.

middleware = createDefaultMiddleware.call(this, connect, options);

My intent in the docs is to document that the default array of middlewares will be used when the value of options.middleware is null or undefined. The subsequent lines in the documentation that you reference provide simple examples of passing a non-null value, either as an Array or as a function.

Does this work for you?

@vladikoff

This comment has been minimized.

Show comment
Hide comment
@vladikoff

vladikoff Feb 19, 2014

Member

Thanks for rebasing the PR. You changed the port from 9xxx to 8xxx, what's the reason for this?

Member

vladikoff commented Feb 19, 2014

Thanks for rebasing the PR. You changed the port from 9xxx to 8xxx, what's the reason for this?

vladikoff added a commit that referenced this pull request Feb 22, 2014

Squashed commit of the following:
commit 1609f28b8033b24aeb9d8ac01bbbfca43a3cd7b3
Author: Steve Jansen <stevejansen_github@icloud.com>
Date:   Wed Feb 19 14:35:16 2014 -0500

    enhance options.middleware to pass the default middleware as an arg

    - modified options.middleware to accept an array or a function

    - modified the arguments sent to the options.middleware function to
      include a third argument that is the array of default middlewares

    - added unit tests for https use cases and new options.middleware
      use cases

    - refactored unit tests and the test case configs in Gruntfile.js to
      use a sequential set of TCP port numbers, starting at 8000,
      for clarity

Closes gh-85.

@vladikoff vladikoff closed this in a06d538 Feb 22, 2014

steve-jansen added a commit to steve-jansen/json-proxy that referenced this pull request Jul 15, 2014

release 0.2.0
Changelog:
* update to http-proxy@1.1.x; http-proxy@1.0 was a "from-scratch" implementation of
  the proxy core by nodejitsu, leaving http-proxy@0.x no longer supported

* implements support for use as grunt middleware;
  includes examples for running a proxy url inside `grunt serve`
  see gruntjs/grunt-contrib-connect#85

* implements support for proxying to SSL/TLS endpoints

* implements support for connecting to HTTP proxy gateways on the
  LAN that require basic authentication credentials

* general housekeeping: added unit tests, resolved jshint issues

steve-jansen added a commit to steve-jansen/json-proxy that referenced this pull request Jul 15, 2014

release 0.2.0
Changelog:
* update to http-proxy@1.1.x; http-proxy@1.0 was a "from-scratch" implementation of
  the proxy core by nodejitsu, leaving http-proxy@0.x no longer supported

* implements support for use as grunt middleware;
  includes examples for running a proxy url inside `grunt serve`
  see gruntjs/grunt-contrib-connect#85

* implements support for proxying to SSL/TLS endpoints

* implements support for connecting to HTTP proxy gateways on the
  LAN that require basic authentication credentials

* general housekeeping: added unit tests, resolved jshint issues

@steve-jansen steve-jansen referenced this pull request in steve-jansen/json-proxy Jul 15, 2014

Closed

Bug in grunt-express config example? #7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment