We shouldn't trust Child Apps in Region — anything could happen ©

maximuk commented 6 years ago

What do you think about adding componentDidCatch method to <Region /> component?

maximuk commented 6 years ago

If we have an error in child app in Region it can potential unmount all the page. So we should use componentDidCatch method in Region component to avoid such scenario.

fahad19 commented 6 years ago

I am yet to dive deeper into the issue, but these come to my mind at the moment:

Possible solution 1

Implement componentDidCatch in your own Child App's root component.

Possible solution 2

When Region renders Child Apps, it can wrap each Child App with a new <RenderApp /> component that we can also ship from the frint-react package:

// Region.js
import React from 'react';

import RenderApp from './App';

export default function Region() {
  const listOfChildApps = [ ... ];

  return (
    <div>
      {listOfChildApps.map((childApp) => {
        return <RenderApp app={childApp} />
      }}
    </div>
  );
}

and here's now the RenderApp component can behave:

// RenderApp.js
import React from 'react';

export default class RenderApp extends React.Component {
  constructor(props) {
    super(props);
    this.state = { hasError: false };
  }

  componentDidCatch(error, info) {
    // Display fallback UI
    this.setState({ hasError: true });

    // callback to Child App
    this.props.app.onComponentError(error, info);
  }

  render() {
    if (this.state.hasError) {
      // You can render any custom fallback UI
      return <h1>Something went wrong.</h1>;
    }
    return theComponentOfChildApp;
  }
}

The Child App itself can be defined like this:

import { createApp } from 'frint';

export default createApp({
  name: 'MyChildApp',

  onComponentError(error, info) {
    // handle it
  }
});

asci commented 6 years ago

Possible solution 3

Implement componentDidCatch wrapper component in website with some sending alerts and wrap all the regions, see ErrorBoundarycomponent here

maximuk commented 6 years ago

Possible solution 1 won't work in case:

import { observe, streamProps } from 'frint-react';

import Root from '../Root';

const RootObserver = () => {
  throw 'error';

  return streamProps({}).get$();
};

export default observe(RootObserver)(Root);

fahad19 commented 6 years ago

@maximuk: what's stopping you to write your Root component like this instead?

// components/Index.js
import React from 'react';

import Root from './Root';

export default class Index extends React.Component {
  constructor(props) {
    super(props);

    this.state = {
      hasError: false
    };
  }

  componentDidCatch(error, info) {
    // Display fallback UI
    this.setState({
      hasError: true
    });
  }

  render() {
    if (this.state.hasError) {
      // You can render any custom fallback UI
      return <h1>Something went wrong.</h1>;
    }

    return <Root />;
  }
}

And set the Index as the main component in your Child App:

// app/index.js
import { createApp } from 'frint';

import Index from '../components/Index';

export default createApp({
  name: 'MyChildApp',

  providers: [
    {
      name: 'component',
      useValue: Index,
    },
  ],
});

See the examples in this PR: https://github.com/Travix-International/travix-appix-templates/pull/6

fahad19 commented 6 years ago

@maximuk: If I don't hear something by Tuesday, I will close this issue assuming the previous comment takes care of your issue.

maximuk commented 6 years ago

Ok, but what if:

// app/index.js
import { createApp } from 'frint';

import Index from '../components/Index';

class ErrorClass {
  constructor() {
    throw 'error';
  }
}

export default createApp({
  name: 'MyChildApp',

  providers: [
    {
      name: 'component',
      useValue: Index,
    },
    {
      name: 'error',
      useClass: ErrorClass,
    },
  ],
});

fahad19 commented 6 years ago

@maximuk: what's the reason of not throwing the error using your custom ErrorClass from inside the componentDidCatch lifecycle hook of your React component?

spzm commented 6 years ago

@fahad19 If child app crashes then it causes crash of root app. Then one small app in the end of the apps tree can break whole application itself.

I suppose that child application won't share errors with core app. And if core app should know about errors of child - then it can subscribe on them.

fahad19 commented 6 years ago

@spzm: in custom setups, Child Apps can log errors originating from their components via cascaded Providers.

an example can be seen here: https://github.com/Travix-International/travix-appix-templates/blob/6bc8e780b7b697e7a2bd3dedf287ff27bdc20d75/default/ui/helloWorldWidget/components/Index.js#L22

mAiNiNfEcTiOn commented 6 years ago

This responsibility cannot be delegated to the app developer. It has to be contained under the core system.

In general, browsers act mostly in the same way when it comes to <script src="..."></script> tags.

<script src="vendors.js"></script>
<script src="a.js"></script>
<script src="b.js"></script>

Say that both a.js and b.js, depend on vendors.js (classical type of structure here)...

If a.js has an error, a.js execution stops, but doesn't block from b.js to run. Vice-versa for b.js

But if by any case, the error happens at the top (vendors.js) because an app is trying to use a method available on the vendors that is not being properly used AND the errors not properly handled, we have a broken page.

Although that should be properly monitored, logged and/or notified to the FB app developer, it should not at any time be a blocker for the whole page. Breaking normal functionality because of a misuse of the FB app dev, it is a big no-no.

fahad19 commented 6 years ago

I see two different scenarios involved here.

When the bundle of Child App itself throws an error

This can be handled at bundler level (Webpack).

Take a look at this example:

// a.js
try {
  (function () {
    // ... bundled child app
  })();
} catch (error) {
  console.log('Something wrong happened in a.js');
}

When error occurs at Child App's component layer

Three use cases need to be handled here:

Catch the error, so that it doesn't break the whole app
Log the error
Render a fallback UI from the Child App when error happens

A solution to this was already suggested in this comment here: https://github.com/frintjs/frint/issues/376#issuecomment-345201379

We need to remember that logging and fallback UI are custom setups. Each application can have its own implementation of logging (or even no logging at all), and Child Apps may decide to handle their fallback UI in their own way.

FrintJS itself does not know which logger provider to use for example, or what Component to render for your specific Child App in case it fails to render.

alexmiranda commented 6 years ago

I fully agree with @fahad19 on this. This is custom setup at least and should not belong in the framework and for lack of a stronger argument, i don't see reasons why to provide an option for error handling when there is one simple and viable option to do it with react already.

We have to keep in mind this has a long-term API impact, so i'd rather handle this error at bundler level or anything outta the framework for that matter.

viacheslaff commented 6 years ago

I don't think Region should be responsible for error handling and specifically for rendering custom UI errors. Nevertheless website should protect itself from crashing caused by child apps. It can be done by creating an ErrorBoundary component, like @asci suggested. It can also be a HOC. Either of those options can be reused for consistent child app error handling in the website. This component can be website specific and I don't think it should be part of frint-react.

mAiNiNfEcTiOn commented 6 years ago

@fahad19

We need to remember that logging and fallback UI are custom setups. Each application can have its own implementation of logging (or even no logging at all), and Child Apps may decide to handle their fallback UI in their own way.

Indeed, but why are we focused on what happens AFTER the exception handling? The logic in storing, doing a fallback or whatever IS custom. But the prevention of a core app from crashing when a child app is broken or triggering a rendering/react error it should be controlled by frint at a higher level, instead of depending on the child app...

@viacheslaff

I don't think Region should be responsible for error handling and specifically for rendering custom UI errors.

When we talk about things like externals or the main thread I can accept this as being specific to the website, indeed. Now here we're talking about specific <script> tags that are breaking. In the concrete example, vendors.js crashing is the sole responsibility of the vendors.js bundle to wrap itself in a try...catch... Why? Because it's a platform dependency, provided by the platform, to be used by the whole ecosystem within the page (root app, child app, etc). And yes, this is business specific, but then again you better have that into consideration when doing the CommonChunks Splitting or Externals' bundle creation.

With that said, again, why isn't it componentDidCatch an enforced method on the App when not specified?

// Simplified logic
if ('componentDidCatch' in this) return;

this.componentDidCatch = () => {
  // captures the error, logs it to error, whatever
};

fahad19 commented 6 years ago

With that said, again, why isn't it componentDidCatch an enforced method on the App when not specified?

Because the packagefrint (which gives us APIs to create Apps) has nothing to do with rendering.

the sole responsibility of the vendors.js bundle to wrap itself in a try...catch... Why?

No one is asking individual developers to wrap their code in a big try/catch. It will be done at our custom bundler-level.

I have a solution in mind that will take care of catching/logging/reporting errors very nicely in our custom bundler-level. we don't need to pollute the framework for this. I will discuss more in details next week.

All of the concerns will be handled.

mAiNiNfEcTiOn commented 6 years ago

@alexmiranda

I fully agree with @fahad19 on this. This is custom setup at least and should not belong in the framework and for lack of a stronger argument, i don't see reasons why to provide an option for error handling when there is one simple and viable option to do it with react already.

AFAIK, frint attempts to be rendering-library agnostic, so therefore a standard way would fit here.

@viacheslaff

Nevertheless website should protect itself from crashing caused by child apps. It can be done by creating an ErrorBoundary component, like @asci suggested. It can also be a HOC. Either of those options can be reused for consistent child app error handling in the website. This component can be website specific and I don't think it should be part of frint-react.

Now, onto the ErrorBoundary suggestion. It's indeed a good one, specially having into account the fact that is suggested at React's blog even: https://reactjs.org/blog/2017/07/26/error-handling-in-react-16.html

Now given the example we see a custom component wrapping a Widget. Let's stop for a second and think which component does that here... Oh wait, that's right, it's the <Region>!

The root app, being the page, if it has an error, it is "acceptable" that doesn't render. Yet, a child app doesn't enjoy the same benefit IMHO.

fahad19 commented 6 years ago

Now given the example we see a custom component wrapping a Widget. Let's stop for a second and think which component does that here... Oh wait, that's right, it's the Region!

You are forgetting that a Region can render multiple Child Apps.

Here's a scenario:

Region named sidebar
Let's say, we catch and render fallback UI from Region component (in frint-react)
Two Child Apps: A and B both targeting sidebar
A fails to render, but B was just fine
Region will catch the error and render a fallback UI by also hiding B (which isn't needed)

That's why I am suggesting all the time that this error catching mechanism should be at Child App's component level. It should be specific, instead of targeting the whole Region.

(Notes below is for an internal application, that others may not understand)

And just like how we inject FB AppID/AppName during bundle time (in our custom bundler), we can wrap a Child App's component with a custom generated ErrorHandlerComponent component, that will implement componentDidCatch, and also log the error appropriately using our custom logger provider.

mAiNiNfEcTiOn commented 6 years ago

@fahad19 who mentioned a fallback UI ? The only thing I'd like to see is the Region swallowing or - if explicitly told - do some processing to the errors, without breaking the page!

What we're seeing here is that you're telling "Yeah, A had an error so I'll just show something" which is completely incorrect. That would be the processing part that should be provided to the region in case we wanted.

Something like:

// somewhere before
myCustomErrorProcessor(error, errorInfo) {
  // do my processing, perhaps logging, perhaps detecting which component failed and unmounting it...
  // but, unless I explicitly want to, does not throw...
}

// in the render of somewhere
<Region name="whatever" onError={myCustomErrorProcessor} />
<Region name="another" />

Where the Region would be:

componentDidCatch(error, info) {
  if (this.props.onError) {
    this.props.onError(error, info);
  }
}

If whatever's child apps have an error, they'll be processed, intentionally processing them.

If another's child apps have an error, they'll just be swallowed.

fahad19 commented 6 years ago

who mentioned a fallback UI ? The only thing I'd like to see is the Region swallowing or - if explicitly told - do some processing to the errors, without breaking the page!

Again, I would like to remind you that Region can render multiple Child Apps. adding an error handler prop at <Region> component level is not a solution.

Region can swallow the error for any Child Apps yes (with componentDidCatch), but it cannot swallow the error for a particular buggy Child App, and still keep rendering other working Child Apps at the same time.

If there is any buggy Child App, we only want to take that particular one down, and know which one it was and log the error appropriately. Others don't need to be taken down with it.

mAiNiNfEcTiOn commented 6 years ago

@fahad19 can you explain to me why are we talking about taking all of them down? I only spoke about swallowing the error. The Region, for all it cares, could not do anything (as the worse thing that can happen is the buggy one not render).

The Fallback UI that you spoke is not even meant to be applied here.

fahad19 commented 6 years ago

I wish it was a choice between taking just one Child App down, or all of them down - directly from a single React Component.

All Child Apps' components are rendered by Region component as siblings. That means, if any of the siblings throw an error, React will automatically fail to render all of the siblings.

To help you understand further, I have created this snippet on JSBin: http://jsbin.com/puxifulifi/edit?html,js,console,output

Two Child components
- Child A: works
- Child B: throws error
One Region component trying to render both Child components
Because of B's error, React fails to render A too at the same time

That's why I keep repeating myself, the error catcher is best placed at individual Child App's component level. Not at Region level, which is supposed to render all the Child Apps together.

And we can put this error catcher nicely in our custom bundler, without doing anything at framework-level here.

mAiNiNfEcTiOn commented 6 years ago

@fahad19 ... In your example it's actually possible... Hackish, but possible:

EDITED: A better approach to control the error without removing the component from the list: http://jsbin.com/rujinihori/6/edit?js,console,output

😉 😆

fahad19 commented 6 years ago

The example has a list of synchronous Child Components.

With FrintJS, the idea is Child Apps can be registered on demand asynchronously, at any time (either on initial page load, or based on some user interactions few seconds or minutes later).

The hackish code will become even more hackish in that case.

If we can keep things simple by doing it at bundler level by keeping our API at framework-level simple, we eliminate the risk of other issues popping up in future.

mAiNiNfEcTiOn commented 6 years ago

Hmmm but @fahad19 , this line - https://github.com/frintjs/frint/blob/fe4c7368153f8642cf4ea467e539330d7d61ae7d/packages/frint-react/src/components/Region.js#L64 - shows an array being used for rendering.

A componentDidCatch() registering on the state the components' names that triggered errors would be enough, no?

I mean, and then filtering the items whose name is not in the state's list of names....

fahad19 commented 6 years ago

That's more state to handle for Region then.

At some point, we will be implementing something like an app.unregisterApp() method, especially when we are going more towards a single-page application, and don't want to hold redundant Child App instances in memory when user has navigated away.

If you add componentDidCatch in Region now, with more stateful logic depending on Child Apps, I can see the level of complexity rise significantly.

mAiNiNfEcTiOn commented 6 years ago

? if you don't want to use the state, you can create a private ~library~ property 😉 and you'd only store Components' names (ergo strings) .... but ok 👍 as you wish sir -.-

spzm commented 6 years ago

I prepare some example http://jsbin.com/xetasefiso/edit?html,js,console,output.

An idea to wrap each component inside Region listForRendering with ErrorHanlder component wrapper that will render null in case of error. It will block any error propagation only for problematic child. Could be a solution?

spzm commented 6 years ago

Also, just notice that componentDidCatch can help only on lifecycles methods errors. In case an error cause outside these methods - componentDidCatch won't work.

https://reactjs.org/docs/error-boundaries.html#how-about-event-handlers

mAiNiNfEcTiOn commented 6 years ago

@spzm exactly, that's why I mentioned the try...catch above 👍 😉 Wrapping the dependencies would solve some of the cases.

Even worse, the way I see it is that you can't even catch your own errors: http://jsbin.com/goyitodaho/1/edit?html,js,console,output

I changed the ChildB to be a class and have the componentDidCatch() and the only method catching it is the parent container (in this case, your ErrorHandler class).

froskie commented 6 years ago

Do we have a conclusion/agreement here? It'd be interesting to know for some guidance.

markvincze commented 6 years ago

@froskie, I think the idea is that we should both

surround the registerApp call with a try-catch to catch any errors during app registration.
implement componentDidCatch to handle any rendering errors.

The work already started in rwd to do this automatically for every app: https://bitbucket.org/xivart/rwd_cheaptickets_nl/pull-requests/5680/mp-223-fb-bundler-handle-errors-from-fb/diff With that in place we don't have to do anything in the actual apps, and we'll get a default implementation of componentDidCatch generated by default, which simply hides the app if its rendering throws an error. @fahad19 what's left to be done in that PR?

fahad19 commented 6 years ago

@markvincze: thanks for explaining so nicely, Mark!

As soon as the acceptance builds are done, and e2e tests are performed, the PR can be merged.

I would suggest doing further (unit/integration) testing-level improvements in our extracted bundler service in future.

I have already given a list of affiliates to @viacheslaff to deploy to acceptance. Best to deploy affiliates which have active Apps as seen in AppSelector UI (in production).

markvincze commented 6 years ago

@viacheslaff do you know when it is realistic for that PR to be merged?

viacheslaff commented 6 years ago

@markvincze, there're problems with unit tests in the pipelines which I'm trying to resolve, so it's hard to tell. Rough estimation would be within a couple of days.

frintjs / frint