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

Add options to enable resilient mode #93

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ncastro-nypr
Copy link

This should fix issue #48.

@kiwiupover
Copy link
Member

@ncastro-nypr I would love to know what resilient mode does and how it works.

@ncastro-nypr
Copy link
Author

ncastro-nypr commented Jan 24, 2019

@kiwiupover Absolutely! It's just a flag on the Fastboot object (See source here). If it is set to true the visit() promise won't be rejected and the result will be returned regardless of errors during rendering.

@ncastro-nypr
Copy link
Author

I lumped in the upgrade to fastboot 2.0.1 into my fork, but for the sake of this PR, it's not important. I can roll that back if there are concerns about upgrading.

@sdhull
Copy link

sdhull commented Jul 9, 2019

@ncastro-nypr thanks for this PR! From what I could tell after testing your branch out locally is that resilient mode still doesn't work as I'd expect, even with being able to pass resilient: true through to Fastboot.

I believe the reason is because of this line in fastboot-express-middleware, which is used under the hood in fastboot-app-server worker.js. When I comment that line out locally (in node_modules 🤠) then resilient mode works as I'd expect. With that line left in-tact, I still get an error page returned from fastboot-app-server.

@sdhull
Copy link

sdhull commented Jul 9, 2019

Also: I think updating Fastboot to 2.0 should be done in a different PR. fastboot-express-middleware has it set to 1.2.0 so this caused both versions to be packaged for me (personally)

@sdhull
Copy link

sdhull commented Jul 10, 2019

@ncastro-nypr ok one more thing, sorry. One thing that would improve this PR is if you could figure out how to spruce up the resilient mode test in here as well. There's a test that is unhelpfully named executes afterMiddleware when there is an error that is kind of about resilient mode (it sets resilient: true but obviously that doesn't do anything).

I've been banging my head against fastboot-express-middleware trying to figure out how it was intending for resilient to be used. There are (slightly) better tests in that repo but I still haven't quite gotten it figured out. I just opened a PR to make those tests a bit clearer. I still haven't figured out why resilient mode seems to work in the middleware repo tests but not here in the fastboot-app-server. I think it expects some sort of error handling middleware to be implemented but I'm not familiar with express.js so not entirely sure.

EDIT: sigh... I wasn't testing it on your branch, so of course it didn't work 🤦‍♂
EDIT2: here is my commit to fix that test. It fails on master but passes on your branch 😅

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.

3 participants