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

Deprecate resilient mode #215

Open
kratiahuja opened this issue Mar 10, 2019 · 11 comments
Open

Deprecate resilient mode #215

kratiahuja opened this issue Mar 10, 2019 · 11 comments
Assignees

Comments

@kratiahuja
Copy link
Contributor

kratiahuja commented Mar 10, 2019

Fastboot supports a resilient mode that allows one to swallow errors and not reject the promise if there are rendering errors. Often this can troll the consumers where they expect some output from fastboot but there was none and no error to indicate what is wrong. This issue is to open discussion whether we should deprecate resilient mode in future.

Alternatively, if we don't want to drop the support, we should atleast add warning when the promise is rejected that fastboot and return the error object as part of promise resolution. This will help consumers understand in more detail.

@rwjblue
Copy link
Member

rwjblue commented Mar 11, 2019

I'm in favor of deprecating.... We should lay out a plan for the deprecation and ultimate removal...

@locks
Copy link

locks commented Mar 13, 2019

Quick RFC? ;)

@rwjblue
Copy link
Member

rwjblue commented Mar 13, 2019

@locks - seems fine either way, happy to review / provide feedback if you do want to write one

@locks locks self-assigned this Apr 15, 2019
@sdhull
Copy link

sdhull commented Jul 8, 2019

My company needs resilient mode to be available.

Shouldn't this issue be more about defaulting resilient to false? Which... after some extensive checking seems to already be done. In fact, it's exceedingly difficult/impossible to set it to true. Maybe something is swallowing errors (and therefore trolling devs) unrelated to resilient??

So the mystery is: what exactly is trolling developers?

@sdhull
Copy link

sdhull commented Jul 8, 2019

A bit more information:
We need resilient mode for occasional network hiccups. We're not really certain what's causing it (or how to repro) but we occasionally get generic "Internal Server Error" pages from fastboot-app-server and logs appear to indicate "adapter aborted" errors as being the cause.

I have no idea how to reproduce an "adapter aborted" error locally, so this has been tough to debug.

However, I believe that resilient mode would largely mitigate this issue for our customers. It remains to be seen how badly google will punish us for returning empty pages in these cases (once in a proverbial blue moon), but it seems like it should be OK. Surely better than "Internal Server Error."

@sdhull
Copy link

sdhull commented Jul 9, 2019

And even more information:
I tried the branch from the PR than passes the resilient flag through in fastboot-app-server and found it still doesn't work. I think it's because internally, fastboot-app-server uses fastboot-express-middleware, and this line in the express middleware does next(result.error); and when I comment that line out then it works as I expect.

@rwjblue
Copy link
Member

rwjblue commented Oct 29, 2019

@sdhull - I still don't fully grok what you need here. Is it that you don't want your express app crashing when there is an error?

@sdhull
Copy link

sdhull commented Oct 29, 2019

Well we're using fastboot-app-server and yes, we do not want error pages served to customers. The errors are often transient and when the page renders client-side it's fine. So I mostly wanted to log my objection to deprecating & removing resilient mode.

These comments were from my mission to get resilient mode working for us in production.
So what I'd recommend for getting resilient mode to work as advertised is to merge ember-fastboot/fastboot-app-server#93 but additionally add docs explaining that resilient mode won't work unless you also add a middleware that skips errors, eg

  ...
  resilient: true,
  afterMiddleware(app) {
    app.use((_err, _req, res, next) => {
      // this is what makes resilient mode actually work :mindblown:
      next();
    });
  },

@sdhull
Copy link

sdhull commented Oct 31, 2019

Oh and it was also to point out that if something is "trolling consumers" as OP mentions, it's not resilient mode, because it's actually impossible to turn resilient mode on. 🤔

@rwjblue
Copy link
Member

rwjblue commented Nov 1, 2019

Gotcha! Thank you for clarifying!

@sdhull - The specific thing that you wanted to do should have been extremely easy, but was not. That is what we want to fix, and resilient mode makes the code much harder and does not make it easier to provide the thing that you are looking for.

Oh and it was also to point out that if something is "trolling consumers" as OP mentions, it's not resilient mode, because it's actually impossible to turn resilient mode on.
...
These comments were from my mission to get resilient mode working for us in production.
So what I'd recommend for getting resilient mode to work as advertised is to merge ember-fastboot/fastboot-app-server#93 but additionally add docs explaining that resilient mode won't work unless you also add a middleware that skips errors, eg
FWIW, this is exactly my definition of trolling people 😝.

My proposal here is to:

  • Deprecate passing resilient as an option in this library (and the other related libs)
  • Add another option (possibly named fallbackToClientRendering?) to fastboot-express-middleware which would add another option to indicate "serve the client renderable HTML page upon any failures" (this is essentially what you had to do manually)

@sdhull
Copy link

sdhull commented Nov 5, 2019

The specific thing that you wanted to do should have been extremely easy

Totally agree. @rwjblue you're basically suggesting we rename resilient to fallbackToClientRendering (and additionally make it easier to use, not forgetting to pass it through in the 3 related addons and not requiring devs to additionally include a middleware that swallows errors). Right?

I think this issue was opened by @kratiahuja to address something entirely different though, which is that sometimes when developing locally using ember-cli-fastboot, errors occur in the fastboot process which are basically invisible in the browser. I'm not sure how to repro this but I feel like I've seen it myself. However I suspect it's not related to resilient mode.

@kratiahuja do you have a way to repro what you're talking about?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants