clean up todos, route request chains, proper 405s

src/client/pjax.ts

@@ -22,9 +22,6 @@ import { StoreValsContext, middleware as storeValsMiddleware} from '../middlewar
 /**
   * This is a helper class to make PJAX easier. PJAX is a clean way of improving the performance of webpages
   * by progressively turning standard HTML links into AJAX requests for portions of a web page.
-  *
-  * @todo Form support
-  * @todo tests
   */
 export default class RoadsPjax {
 	protected _road: Road;
@@ -73,7 +70,6 @@ export default class RoadsPjax {
 
 			return next().then((response) => {
 				if (this.getVal) {
-					// TODO: I'm not sure I like title key in here. Maybe we should have it passed in?
 					pjaxObj._page_title = this.getVal(titleKey) as string;
 				}
 
@@ -91,8 +87,7 @@ export default class RoadsPjax {
 	 * This function call enables PJAX on the current page.
 	 */
 	register (): void {
-		// Handle navigation changes besides pushState. TODO: don' blow out existing onpopstate's
-		// TODO: If a request is in process during the popstate, we should kill it and use the new url
+		// Handle navigation changes besides pushState.
 		this._window.onpopstate = (event: PopStateEvent) => {
 			if (event.state.pjax) {
 				// if the popped state was generated  via pjax, execute the appropriate route
@@ -156,7 +151,6 @@ export default class RoadsPjax {
 		if (event.target instanceof HTMLAnchorElement && event.target.dataset['roadsPjax'] === 'link' && !event.ctrlKey) {
 			event.preventDefault();
 			this._roadsLinkEvent(event.target as HTMLAnchorElement);
-			// TODO: Change this to a on submit event?
 		} else if ((event.target instanceof HTMLInputElement || event.target instanceof HTMLButtonElement)
 					&& event.target.dataset['roadsPjax'] === 'submit'
 					&& event.target.form && event.target.form.dataset['roadsPjax'] === 'form') {
@@ -205,7 +199,6 @@ export default class RoadsPjax {
 		)
 			.then((response: Response) => {
 				if ([301, 302, 303, 307, 308].includes(response.status) && typeof response.headers?.location === 'string') {
-					// todo: location can be an array via code, but I don't think it's vaild to the spec?
 					return this._road.request('GET', response.headers.location);
 				} else {
 					return response;

src/client/request.ts

@@ -14,17 +14,13 @@ import Response from '../core/response';
  * This class is a helper with making HTTP requests that look like roads requests.
  * The function signature matches the roads "request" method to allow the details of a request to be abstracted
  * away from the client. Sometimes the request may route internally, sometimes it may be an HTTP request.
- *
- * @todo tests
  */
 export default class Request {
 	protected secure: boolean;
 	protected host: string;
 	protected port: number;
 
 	/**
-	 * @todo: port should just be part of the host
-	 *
 	 * @param {boolean} secure - Whether or not this request should use HTTPS
 	 * @param {string} host - The hostname of all requests made by this function
 	 * @param {number} port - The post of all requests made by this function
@@ -76,9 +72,6 @@ export default class Request {
 			headers: newHeaders,
 			redirect: 'manual',
 			referrerPolicy: 'no-referrer',
-
-			// TODO: What should we do with these?
-			//   origin-when-cross-origin, same-origin, strict-origin, strict-origin-when-cross-origin, unsafe-url
 			body
 		});
 

src/core/requestChain.ts

@@ -0,0 +1,61 @@
+/* eslint-disable max-len */
+import Response from './response';
+import { Context } from './road';
+
+export interface NextCallback {
+	// eslint-disable-next-line @typescript-eslint/no-explicit-any
+	(): Promise<Response | string>
+}
+
+// eslint-disable-next-line @typescript-eslint/ban-types
+export class RequestChain<fn extends Function> {
+	/**
+	 * The request chain is an array of functions that are executed in order when `run` is called
+	 */
+	protected _function_chain: fn[];
+
+	/**
+	 * RequestChain Constructor
+	 *
+	 * Creates a new RequestChain object
+	 */
+	constructor (initial?: fn[]) {
+		this._function_chain = initial || [];
+	}
+
+	add (fn: fn) {
+		this._function_chain.push(fn);
+	}
+
+	length () {
+		return this._function_chain.length;
+	}
+
+	/**
+	 * The `run` function is used to execute the function chain.
+	 *
+	 * The function chain is executed in order, with each function in the chain being called with the parameters provided to the function.
+	 *
+	 * The function function returns a Promise that resolves to a Response object or a string.
+	 *
+	 */
+	getChainStart () {
+
+		let progress = 0;
+		const next: (context: Context, ...args: any[]) => Promise<Response | string> = async (context, ...args) => {
+
+			if (this._function_chain.length && this._function_chain[progress]) {
+				return this._function_chain[progress].call(context, ...args, () => {
+					progress += 1;
+					return next(context, ...args);
+				});
+			}
+
+			// If next is called and there is nothing next, we should still return a promise,
+			//		it just shouldn't do anything
+			return new Response('Page not found', 404);
+		};
+
+		return next;
+	}
+}

src/core/road.ts

@@ -9,7 +9,7 @@
 
 import * as response_lib from './response';
 import Response from './response';
-
+import { NextCallback, RequestChain } from './requestChain';
 
 export interface IncomingHeaders extends Record<string, string | Array<string> | undefined> {}
 
@@ -19,10 +19,6 @@ export interface Middleware<MiddlewareContext extends Context> {
 		next: NextCallback): Promise<Response | string> | Response | string
 }
 
-export interface NextCallback {
-	(): Promise<Response | string>
-}
-
 export interface Context extends Record<string, unknown> {}
 
 /**
@@ -31,15 +27,15 @@ export interface Context extends Record<string, unknown> {}
  * @name Road
  */
 export default class Road {
-	protected _request_chain: Middleware<Context>[];
+	protected _request_chain: RequestChain<Middleware<Context>>;
 
 	/**
 	 * Road Constructor
 	 *
 	 * Creates a new Road object
 	 */
 	constructor () {
-		this._request_chain = [];
+		this._request_chain = new RequestChain();
 	}
 
 	/**
@@ -73,7 +69,7 @@ export default class Road {
 	use<ContextType extends Context> (fn: Middleware<ContextType>): Road {
 		// Currently we pass everything through the coroutine wrapper to be save. Let that library decide
 		// 		what does and does not actually need to be wrapped
-		this._request_chain.push(fn);
+		this._request_chain.add(fn);
 
 		return this;
 	}
@@ -94,40 +90,6 @@ export default class Road {
 	 * @returns {Promise} this promise will resolve to a Response object
 	 */
 	request (method: string, url: string, body?: string, headers?: IncomingHeaders): Promise<Response> {
-		return response_lib.wrap(this._buildNext({}, method, url, body, headers)());
-	}
-
-	/**
-	 * Turn an HTTP request into an executable function with a useful request context. Will also incorporate the entire
-	 * request handler chain
-	 *
-	 * @param {Context} context - Request context
-	 * @param {string} request_method - HTTP request method
-	 * @param {string} path - HTTP request path
-	 * @param {string} request_body - HTTP request body
-	 * @param {object} request_headers - HTTP request headers
-	 * @returns {NextMiddleware} A function that will start (or continue) the request chain
-	 */
-	protected _buildNext (context: Context, request_method: string, path: string, request_body?: string,
-		request_headers?: IncomingHeaders
-	): NextCallback {
-
-		let progress = 0;
-		const next: NextCallback = async () => {
-
-			if (this._request_chain.length && this._request_chain[progress]) {
-				return this._request_chain[progress].call(context,
-					request_method, path, request_body, request_headers, () => {
-						progress += 1;
-						return next();
-					});
-			}
-
-			// If next is called and there is nothing next, we should still return a promise,
-			//		it just shouldn't do anything
-			return new Response('Page not found', 404);
-		};
-
-		return next;
+		return response_lib.wrap(this._request_chain.getChainStart()({}, method, url, body, headers));
 	}
 }

src/middleware/applyToContext.ts

@@ -12,15 +12,12 @@ import { Context, Middleware } from '../core/road';
 /**
  * This is a very simple middleware to apply a single value to the request context with a single line of code.
  *
- * TODO: Get better typing on this. I think we need to wait for https://github.com/Microsoft/TypeScript/pull/26797.
- *		In the meanwhile if you use this middleware you should create your own context.
- *
  * @param {string} key - The key that should store the value on the request context.
  * @param {any} val - The value to apply to the request context.
  * @returns {Middleware} The middleware function to apply to the road.use(fn) method.
  */
-export function build (key: string, val: unknown): Middleware<Context> {
-	const applyToContext: Middleware<Context> = function (method, url, body, headers, next) {
+export function build<T extends Context, K extends keyof T> (key: K, val: T[K]): Middleware<T> {
+	const applyToContext: Middleware<T> = function (method, url, body, headers, next) {
 		this[key] = val;
 		return next();
 	};

src/middleware/basicRouter.ts

@@ -8,18 +8,19 @@
  */
 
 import parse from 'url-parse';
-import { IncomingHeaders, NextCallback } from '../core/road';
+import { IncomingHeaders } from '../core/road';
 import Road, {Context} from '../core/road';
 import Response from '../core/response';
+import { NextCallback, RequestChain } from '../core/requestChain';
 
 
 export interface Route<ContextType extends Context> {
-	(this: ContextType, path: BasicRouterURL, body: string,
+	(this: ContextType, method: string, path: BasicRouterURL, body: string,
 		headers: IncomingHeaders, next: NextCallback): Promise<Response>
 }
 
 interface RouteDetails {
-	route: Route<Context>,
+	route: Route<Context> | RequestChain<Route<Context>>,
 	path: string,
 	method: string
 }
@@ -98,7 +99,9 @@ export class BasicRouter {
 	 * @param {(string|array)} paths - One or many URL paths that will trigger the provided function
 	 * @param {function} fn - The function containing all of your route logic
 	 */
-	addRoute<ContextType extends Context> (method: string, paths: string | string[], fn: Route<ContextType>): void {
+	addRoute<ContextType extends Context> (
+		method: string, paths: string | string[], fn: Route<ContextType> | Route<ContextType>[]
+	): void {
 		if (!Array.isArray(paths)) {
 			paths = [paths];
 		}
@@ -107,7 +110,7 @@ export class BasicRouter {
 			this._routes.push({
 				path: path,
 				method: method,
-				route: fn
+				route: Array.isArray(fn) ? new RequestChain(fn) : fn
 			});
 		});
 	}
@@ -137,16 +140,17 @@ export class BasicRouter {
 	 * Slightly non-standard roads middleware to execute the functions in this router when requests are received by the road
 	 * The first method is the routes to ensure that we can properly use this router once we loose the "this" value
 	 * from the roads context
-	 *
-	 * @todo there might be a better way to do this
 	 */
-	protected _middleware (routes: RouteDetails[], request_method: string, request_url: string, request_body: string,
-		request_headers: IncomingHeaders, next: NextCallback): Promise<Response | string> {
+	protected async _middleware (
+		this: Context, routes: RouteDetails[], request_method: string, request_url: string, request_body: string,
+		request_headers: IncomingHeaders, next: () => Promise<Response | string>
+	): Promise<Response | string> {
 
 		let realMethod = request_method;
 
 		let response = null;
 		let hit = false;
+		let routeHitMethodFail = false;
 
 		const parsed_url = parse(request_url, true);
 
@@ -165,17 +169,31 @@ export class BasicRouter {
 		for (let i = 0; i < routes.length; i++) {
 			const route = routes[i];
 
-			if (compareRouteAndApplyArgs(route, parsed_url, realMethod)) {
-				response = (route.route).call(this, parsed_url, request_body, request_headers, next);
-				hit = true;
-				break;
+			if (compareRouteAndApplyArgs(route, parsed_url)) {
+				// Check the method last, so we can give proper status codes
+				if (route.method === realMethod) {
+					if (route.route instanceof RequestChain) {
+						response = route.route.getChainStart()(this, realMethod, parsed_url, request_body, request_headers);
+					} else {
+						response = (route.route).call(this, realMethod, parsed_url, request_body, request_headers, next);
+					}
+
+					hit = true;
+					break;
+				} else {
+					routeHitMethodFail = true;
+				}
 			}
 		}
 
 		if (hit) {
 			return response;
 		}
 
+		if (routeHitMethodFail) {
+			return new Response('Method Not Allowed', 405);
+		}
+
 		return next();
 	}
 }
@@ -190,10 +208,9 @@ export class BasicRouter {
  * @param {string} request_method - HTTP request method
  * @returns {boolean}
  */
-function compareRouteAndApplyArgs (route: {method: string, path: string}, request_url: ReturnType<typeof parse>,
-	request_method: string): boolean {
+function compareRouteAndApplyArgs (route: {method: string, path: string}, request_url: ReturnType<typeof parse>): boolean {
 
-	if (route.method !== request_method || !request_url.pathname) {
+	if (!request_url.pathname) {
 		return false;
 	}
 
@@ -270,7 +287,6 @@ function applyArg(request_url: BasicRouterURL, template_part: string, actual_par
 /**
  * Applies a prefix to paths of route files
  *
- * @todo I'm pretty sure there's an existing library that will do this more accurately
  * @param {string} path - The HTTP path of a route
  * @param {string} [prefix] - An optional prefix for the HTTP path
  * @returns {string}

src/middleware/cookieMiddleware.ts

@@ -80,7 +80,6 @@ function (route_method, route_path, route_body, route_headers, next) {
 	// Find the cookies from the request and store them locally
 	if (route_headers && route_headers.cookie) {
 		cookies = cookie.parse(
-			// todo: hmm... Can we get an array of cookies? I don't think so... this handles it properly if we do though.
 			Array.isArray(route_headers.cookie) ? route_headers.cookie.join('; ') : route_headers.cookie);
 	}
 
@@ -108,15 +107,18 @@ function (route_method, route_path, route_body, route_headers, next) {
 				response = new Response(response);
 			}
 
-			// Initalize the header
+			// Force the set cookie response header to be an array, for ease of applying them below.
 			if (!response.headers['Set-Cookie']) {
 				response.headers['Set-Cookie'] = [];
 			}
 
+			if (typeof response.headers['Set-Cookie'] === 'string') {
+				response.headers['Set-Cookie'] = [response.headers['Set-Cookie']];
+			}
+
 			// Apply all the cookies
 			for (let i = 0; i < cookieKeys.length; i++) {
-				// TODO: Don't have this "as"
-				(response.headers['Set-Cookie'] as Array<string>).push(
+				(response.headers['Set-Cookie']).push(
 					cookie.serialize(cookieKeys[i],
 						this.newCookies[cookieKeys[i]].value, this.newCookies[cookieKeys[i]].options));
 			}