Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address routr api changes & Handle POST #837

Merged
merged 15 commits into from
Jan 18, 2017
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/page-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ A binding of URL paths to a Page class that can render the results for those URL

* The HTTP method(s) that are acceptable.

* Default: “get”
* Default: [“get”, "head"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly necessary and if people disagree, it can easily be changed back. I like to have parity between GET/HEAD in most cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does react-server even understand head, or does it just return the same thing as with get? (You'd think I'd know the answer to this...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... is it common to have an API respond to, e.g. POST or HEAD? (i.e., should we be auto-adding head, to everything-ish?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it stands right now, react-server throws a 404 error for all HEAD requests because routes being passed to routr don't send the method attribute with it. Therefore, routr defaults to 'GET' and checks the request. head !== get, therefore there's no route found for HEAD/POST/PUT/PATCH/etc.


`page: Page`
* The page to render
Expand Down
13 changes: 8 additions & 5 deletions packages/react-server-cli/src/compileClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -291,14 +291,17 @@ module.exports = {
for (let routeName of Object.keys(routes.routes)) {
let route = routes.routes[routeName];

// if the route doesn't have a method, we'll assume it's ["get", "head"]. routr doesn't
// have a default (method is required), so we set it here.
if (!route.method) {
route.method = ["get", "head"];
}

routesOutput.push(`
${routeName}: {`);
routesOutput.push(`
path: ${JSON.stringify(route.path)},`);
// if the route doesn't have a method, we'll assume it's "get". routr doesn't
// have a default (method is required), so we set it here.
routesOutput.push(`
method: "${route.method || "get"}",`);
path: ${JSON.stringify(route.path)},
method: ${JSON.stringify(route.method)},`);

let formats = normalizeRoutesPage(route.page);
routesOutput.push(`
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export default class DumbPage {
getElements() {
const routeName = this.getRequest().getRouteName();
return <div id="routeName">{routeName}</div>;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import DumbPage from "./DumbPage";

// This wrapper is taken from the compileClient function that writes the routes_(client|server).js file
const pageWrapper = {
default: () => {
return {
done: (cb) => { cb(DumbPage); },
};
},
};

const Routes = {
"BasicPage": {
"path": ["/basicPage"],
"page": pageWrapper,
},
"BasicPageCaps": {
"path": ["/basicPageCaps"],
"page": pageWrapper,
"method": "GET", // this should be all caps because the req.getMethod() will return 'get'.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fact that case sensitivity matters smells to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is testing that case sensitivity doesn't matter in the Routr package. In versions <0.1.1, it did a case sensitive match. >=0.1.1, it added a toLowerCase() call first. We want to make sure that doesn't regress.

},
"PostPage": {
"path": ["/postPage"],
"page": pageWrapper,
"method": "post",
},
"GetAndPostPage": {
"path": ["/getAndPostPage"],
"page": pageWrapper,
"method": ["get", "post"],
},
};

export default Routes;
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
import Navigator from "../../../context/Navigator";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THANK YOU FOR THESE TESTS!

import History from "../../../components/History";
import ExpressServerRequest from "../../../ExpressServerRequest";
import NavigatorRoutes from "./NavigatorRoutes";

class RequestContextStub {
constructor(options) {
this.navigator = new Navigator(this, options);
}
navigate (request, type) {
this.navigator.navigate(request, type);
}
framebackControllerWillHandle() { return false; }
getMobileDetect() { return null; }
}


describe("Navigator", () => {
let requestContext;
const options = {
routes: NavigatorRoutes,
};

beforeEach(() => {
//requestContext = new RequestContext(options);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't leave in commented-out code without an explanation

requestContext = new RequestContextStub(options);
});
afterEach(() => {
requestContext = null;
});

it("routes to a basic page using an unspecified method get", (done) => {
const req = {
method: "get",
protocol: "http",
secure: false,
hostname: "localhost",
url: "/basicPage",
};
const expressRequest = new ExpressServerRequest(req);

requestContext.navigator.on('page', () => {
expect(expressRequest.getRouteName() === 'BasicPage');
done();
});

requestContext.navigate(expressRequest, History.events.PAGELOAD);
});

it("routes to a basic page using an unspecified method HEAD", (done) => {
const req = {
method: "head",
protocol: "http",
secure: false,
hostname: "localhost",
url: "/basicPage",
};
const expressRequest = new ExpressServerRequest(req);

requestContext.navigator.on('page', () => {
expect(expressRequest.getRouteName() === 'BasicPage');
done();
});

requestContext.navigate(expressRequest, History.events.PAGELOAD);
});

it("routes to a page using a method GET (caps)", (done) => {
const req = {
method: "get",
protocol: "http",
secure: false,
hostname: "localhost",
url: "/basicPageCaps",
};
const expressRequest = new ExpressServerRequest(req);

requestContext.navigator.on('page', () => {
expect(expressRequest.getRouteName() === 'BasicPageCaps');
done();
});

requestContext.navigate(expressRequest, History.events.PAGELOAD);
});

it("routes to a page using a method POST", (done) => {
const req = {
method: "post",
protocol: "http",
secure: false,
hostname: "localhost",
url: "/postPage",
};
const expressRequest = new ExpressServerRequest(req);

requestContext.navigator.on('page', () => {
expect(expressRequest.getRouteName() === 'PostPage');
done();
});

requestContext.navigate(expressRequest, History.events.PAGELOAD);
});


it("routes to a page that can handle GET and POST using a method GET", (done) => {
const req = {
method: "get",
protocol: "http",
secure: false,
hostname: "localhost",
url: "/getAndPostPage",
};
const expressRequest = new ExpressServerRequest(req);

requestContext.navigator.on('page', () => {
expect(expressRequest.getRouteName() === 'GetAndPostPage');
done();
});

requestContext.navigate(expressRequest, History.events.PAGELOAD);
});

it("routes to a page that can handle GET and POST using a method POST", (done) => {
const req = {
method: "post",
protocol: "http",
secure: false,
hostname: "localhost",
url: "/getAndPostPage",
};
const expressRequest = new ExpressServerRequest(req);

requestContext.navigator.on('page', () => {
expect(expressRequest.getRouteName() === 'GetAndPostPage');
done();
});

requestContext.navigate(expressRequest, History.events.PAGELOAD);
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some failure cases here too? E.g. doesn't respond to a POST request when configured with PUT only &c.

2 changes: 1 addition & 1 deletion packages/react-server/core/context/Navigator.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class Navigator extends EventEmitter {

this._haveInitialized = true;

var route = this.router.getRoute(request.getUrl(), {navigate: {path:request.getUrl(), type:type}});
var route = this.router.getRoute(request.getUrl(), {method: request.getMethod()});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found myself wondering: how come we don't need type in here anymore? And then I found myself wondering: why did we need it in the first place?

The only thing I can think of offhand that might break here, is if type is being stored on the route object that's returned and we're reaching into it instead of using the type we're passing around everywhere.

I looked at the commit in routr, and did some grepping, and I think we're probably in the clear. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confident in all of the changes here except this type thing. I really don't understand it at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why it was necessary either, since it didn't really seem to be used. It might have been used with fluxible, which routr was originally built for. But we basically picked the History API and routr from that project (and then modified the history API, IIRC) and called it a day. So it's not particularly surprising that we weren't using the extras on route.


if (route) {
logger.debug(`Mapped ${request.getUrl()} to route ${route.name}`);
Expand Down