search

ember-fastboot / fastboot-app-server

A production-ready app server for running Ember FastBoot apps
140 stars 74 forks source link

Add options to enable resilient mode #93

Open ncastro-nypr opened 5 years ago

ncastro-nypr commented 5 years ago

This should fix issue #48.

kiwiupover commented 5 years ago

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

ncastro-nypr commented 5 years ago

@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 commented 5 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 😅