include html in Express render callback #271
Conversation
|
@marudor looks like something's up with the Travis config https://travis-ci.org/flowtype/flow-typed/builds/154836301 |
| @@ -73,7 +73,7 @@ declare module 'express' { | |||
| location(path: string): this; | |||
| redirect(url: string, ...args: Array<void>): this; | |||
| redirect(status: number, url: string, ...args: Array<void>): this; | |||
| render(view: string, locals?: mixed, callback: (err?: ?Error) => mixed): this; | |||
| render(view: string, locals?: mixed, callback: (err?: ?Error, html: string) => mixed): this; | |||
There was a problem hiding this comment.
Nit: This type is a little odd. Flow probably should be changed to ban non-optional parameters from following optional parameters.
If the callback method is always passed two arguments
err, which is either an instance ofErrorornullhtml, which is a string
then the most correct type would be (err: Error | null, html: string) => mixed).
However, as written, this is basically (err: Error | null | void, html: string) => mixed), which is only a little bit more strict than it needs to be.
So I'm happy to merge without this being fixed. Just let me know what you want to do!
There was a problem hiding this comment.
Good catch. Looks like html is omitted if there's an error: https://github.com/expressjs/express/blob/master/lib/application.js#L581
And it's always called with at least the first arg. So I'll change the signature.
The Response render just calls to app render so I'll share the type too.
|
Not sure what's going on with travis, but this looks good to me! I have one nit, which I mentioned inline, but happy to merge even without it being fixed. |
|
Good catch on the callback being called sometimes with only one argument! I think you might need to update the test too, though |
|
Looks good! Thanks for bearing with me and thanks for your contribution! |
|
btw, what's your current philosophy on aligning with definitelytyped? (facebook/flow#7) Seems a waste to write all these types twice. I've heard Facebook wants a flow2typescript rather than typescript2flow because Flow can express more. Seems like it would be easier to make a subset work in a superset than reduce that extra expressivity, but I don't know much of the details here. Before either one, could the two repos be made to support the same test files? That's a lot of the work right there, figuring out what the spec is for these JS libs. |
http://expressjs.com/en/api.html#res.render