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

Adds new render option #7

Closed
wants to merge 9 commits into from
Closed

Adds new render option #7

wants to merge 9 commits into from

Conversation

gtomitsuka
Copy link

I developed this function for myself because I need it for a project I'm working on, but it basically adds a way for finalhandler's users' to do custom rendering of their views.

The test/test.js passes, and I think the README.md and test/test.js show how to work with it properly.

@dougwilson
Copy link
Contributor

This module does not do much besides simply render. Why even invoke this module at all instead of defining error handlers in Express?

@dougwilson dougwilson added the pr label Jul 16, 2015
@dougwilson dougwilson self-assigned this Jul 16, 2015
@@ -104,7 +106,12 @@ function finalhandler(req, res, options) {
return req.socket.destroy()
}

send(req, res, status, msg)
if(typeof render === 'function'){
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling finalhandler(req, res, {render: 42}) needs to throw an error, not silently pretend like the option was not there.

@dougwilson
Copy link
Contributor

So besides the question and the two notes, I am working on getting the 1.x branch ported to master for the Express 5.0 release next week. I updated the 1.x branch to be based on 0.4.0 and already moved one of the commits onto master. If you can possibly rebase this PR onto the old 1.x branch (I see you have that branch in your fork already), that would help a lot as well :)!

@gtomitsuka
Copy link
Author

Oops :P

@gtomitsuka
Copy link
Author

@dougwilson I would like usable 404 handlers working without adding additional handlers, and I think other people want it, too. And I'll make the changes.

@dougwilson
Copy link
Contributor

I would like usable 404 handlers working without adding additional handlers

What does that mean? Remember, this module is what Express uses only if you don't provide anything. Are you sure you are not imposing Express questions/issues onto this module?

@dougwilson
Copy link
Contributor

The other weird part of your statement regarding 404 handlers here is that the render option you have in this PR only works for errors; it doesn't have anything to do with 404 handling. Perhaps your render is working on the wrong part? I'm really, really confused and I'm probably going to say that I need you to provide your full, complete ultimate use-case you are trying to achieve with this PR before we can proceed here so I know this is the right solution.

@gtomitsuka
Copy link
Author

First of all, for this one project I am using finalhandler() directly.

Now, the render works for both errors and 404s. When calling done() without an error, the message appears:

screen shot 2015-07-17 at 12 04 48 pm

I use finalhandler for both errors and 404 messages. With the render option

I thought of it because I wanted a catcher for 404 errors, but it's not only for 404 errors. With the render function people will be able to actually use the Express/Connect error messages(like Express' try catch) on production.

@dougwilson
Copy link
Contributor

Now, the render works for both errors and 404s.

Gotcha. I don't see a test in your PR to test using render with 404s. Can you add it?

@gtomitsuka
Copy link
Author

Sure.

@gtomitsuka
Copy link
Author

(I'm busy today, I'll do it tomorrow)

@dougwilson
Copy link
Contributor

So there hasn't been any update for a couple years and the original repo is gone, so I cannot merge. Please feel free to open it up again if you would like to resume.

@dougwilson dougwilson closed this Sep 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants