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

Export "app" #8403

Merged
merged 2 commits into from Mar 8, 2017
Merged

Export "app" #8403

merged 2 commits into from Mar 8, 2017

Conversation

domq
Copy link
Contributor

@domq domq commented Feb 21, 2017

As per the comment on line 653, this appears to be the best (only?) way to let users do things like set up a custom error page to catch app-rendering-time exceptions.

This purports to solve #8402

As per the comment on line 653, this appears to be the best (only?) way to let users do things like set up a custom error page to catch app-rendering-time exceptions.
@apollo-cla
Copy link

@domq: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@abernix
Copy link
Contributor

abernix commented Feb 22, 2017

Not entirely opposed to this but is this something you weren't able to accomplish using WebApp.connectHandlers.use?

A little more insight into what you've tried might be helpful here.

@stubailo
Copy link
Contributor

Yeah rendering exceptions should be able to be caught with an error middleware on connectHandlers.

@glasser
Copy link
Contributor

glasser commented Mar 1, 2017

As the comment that @domq alluded to says, connect (at least the version Meteor uses) does not recursively search into nested middleware to search for 4-argument error middleware.

@abernix
Copy link
Contributor

abernix commented Mar 1, 2017

Ok, so based on what @glasser is saying, and his own comment here I can understand more now why this is necessary. I hadn't remembered that was the case.

Given that, I'm in favor of this change, however I think we should consider a different name. Currently the proposed API would be WebApp.connectMiddleware, however, this is specifically the "app" middleware, I believe.

Should we call it WebApp.appMiddleware? Or just WebApp.middleware to avoid the redundancy?

EDIT: Actually, maybe this should just be WebApp.app? It's not really middleware at this point, is it? WebApp.server?

@hwillson
Copy link
Contributor

hwillson commented Mar 2, 2017

How about WebApp.connectApp? That's what we're really exposing and connect themselves refer to the container that holds the defined middleware (the result of calling the connect() function) as a connect "app". I realize we might not want to expose internal details in an externally available property name (in-case connect is removed at some point), but hey we're already doing that with WebApp.connectHandlers ... 🙂

@abernix
Copy link
Contributor

abernix commented Mar 8, 2017

Any thoughts, @domq?

@domq
Copy link
Contributor Author

domq commented Mar 8, 2017 via email

Because as @hwillson rightfully points out:

> that's what we're really exposing and `connect` themselves refer to the container that holds the defined middleware (the result of calling the `connect()` function) as a connect "app"."
@abernix abernix added this to the Release 1.4.3.x milestone Mar 8, 2017
Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

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

LGTM!

@abernix abernix merged commit 0281012 into meteor:devel Mar 8, 2017
@abernix
Copy link
Contributor

abernix commented Mar 8, 2017

Never hurts to ask for another opinion, thanks for the response! And no worries, I've gone ahead and updated it (via GitHub Edit).

Thanks, @domq.

@abernix abernix modified the milestones: Release 1.4.3.x, Release 1.4.3.2 Mar 9, 2017
@domq domq deleted the patch-1 branch May 4, 2017 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants