feat(error-boundary): add ErrorBoundaryGroup #157

manudeli · 2022-11-24T19:51:58Z

Overview

Referencing Issue

I Added ErrorBoundary.ResetKey, ResetKey, useResetKey, withResetKey to reset ErrorBoundary outside of itself easily.

Problem

Resetting ErrorBoundary outside of itself is tiresome

const Example = () => {
  const [key, setKey] = useState(0)
  const resetKey = () => setKey(prev => prev + 1)

  return <>
    <NestedResetter resetKey={resetKey} />
    <ErrorBoundary resetKeys={[key]}>
      <ThrowComponent>
    </ErrorBoundary>
    <NestedErrorBoundary resetKey={resetKey} /> {/* prop drilling is necessary */}
  </>
}

Solution

if use useResetKey or ErrorBoundary.ResetKey, prop drilling is not necessary.

const Example = withResetKey(() => (
  <>
    <NestedResetter />
    <ErrorBoundary.ResetKey>
      <ThrowComponent>
    </ErrorBoundary>
    <NestedErrorBoundary /> {/* prop drilling is not necessary */}
  </>
))

How to use

Using withResetKey

const Example = withResetKey(({ reset: resetAtOnce, resetKey }) => {
  const [ifYouWantToAddOtherDependency] = useState(false);

  return (
    <>
      <NestedResetter /> {/* Accessing ResetKeyContext is available by using useResetKey */}

      <button onClick={resetAtOnce}>reset at once outside of ErrorBoundary</button> {/* Resetting is easy */}

      <ErrorBoundary.ResetKey
        resetKeys={[ifYouWantToAddOtherDependency]} // Adding any other resetKey in resetKeys of ErrorBoundary.ResetKey is available
        renderFallback={({ reset }) => <button onClick={reset}>reset only self</button>}
      >
        <ThrowComponent />
      </ErrorBoundary.ResetKey>

      <ErrorBoundary
        resetKeys={[resetKey]}
        renderFallback={({ reset }) => <button onClick={reset}>reset only self</button>}
      >
        <ThrowComponent />
      </ErrorBoundary>
    </>
  );
});

Nested Component using useResetKey

const NestedResetter = () => {
  const { reset } = useResetKey();

  return <button onClick={reset}>reset ErrorBoundary outside of ErrorBoundary</button>;
};

Using ResetKey.Provider, ResetKey.Consumer

const Example = () => (
  <ResetKey.Provider>
    <NestedResetter />
    <ResetKey.Consumer>
      {({ resetKey }) => (
        <ErrorBoundary
          resetKeys={[resetKey]}
          renderFallback={({ reset, error }) => <button onClick={reset}>reset {JSON.stringify(error)}</button>}
        >
          <ThrowComponent />
        </ErrorBoundary>
      )}
    </ResetKey.Consumer>
  </ResetKey.Provider>
);

or Using ResetKey.Provider, ErrorBoundary.ResetKey

const Example = () => (
  <ResetKey.Provider>
    <NestedResetter />
	<ErrorBoundary.ResetKey
      renderFallback={({ reset, error }) => <button onClick={reset}>reset {JSON.stringify(error)}</button>}
    >
      <ThrowComponent />
    </ErrorBoundary.ResetKey>
  </ResetKey>
);

PR Checklist

I read and included theses actions below

I have read the Contributing Guide
I have written documents and tests, if needed.

netlify · 2022-11-24T19:52:01Z

👷 Deploy request for slash-libraries pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`b12ec9c`

…etKey

raon0211 · 2022-11-28T02:24:36Z

Thanks for your pull request, @manudeli. Now I see the problem. However I see some problems as below:

I think ErrorBoundary.ResetKey and withResetKey exposes implementation detail to its public API. I think we should only expose its intended behavior to the public API.
- What we expect for withResetKey is to define a shared boundary reset context. so in this case withErrorBoundaryContext or withErrorBoundaryGroup might be a better naming.
Why should ErrorBoundary.ResetKey and BaseErrorBoundary be separated? Why not merging them into one, i.e. ErrorBoundary? (What about defining a default reset key and making the default ErrorBoundary use it in case it has no providers? It wouldn't break the existing code and will make our codebase easier to maintain)
I think this is a new feature introduced in slash libraries, and we think new features should have a test case. Can you add a test case in this pull request? If not, we could add some test cases in this pull request.

raon0211 · 2022-11-28T02:26:08Z

packages/react/error-boundary/src/index.ts

@@ -1,3 +1,4 @@
 /** @tossdocs-ignore */
-export { default as ErrorBoundary } from './ErrorBoundary';
+export { ResetKey, useResetKey, withResetKey } from './contexts/ResetKey';


As noted in the earlier comment, I think

ErrorBoundaryContext & withErrorBoundaryContext

ErrorBoundaryGroup & withErrorBoundaryGroup

might be a better naming.

In case of useResetKey, what about useErrorBoundaryContext ?

const context = useErrorBoundaryContext(); context.reset();

I think resetKey is implementation detail and I would like to hide it from the public API.

manudeli · 2022-11-28T12:33:05Z

ErrorBoundaryGroup

I changed ResetKey into ErrorBoundaryGroup

If ErrorBoundaryGroup as parent of ErrorBoundary

now, default ErrorBoundary can be reset by ErrorBoundaryGroup's groupResetKey of ErrorBoundaryGroup context in background.

also, I named trigger resetting ErrorBoundaryGroup as ErrorBoundaryGroup.Reset that can combine with every component a feature resetting all ErrorBoundary as ErrorBoundaryGroup's children.

const Example = () => (
  <ErrorBoundaryGroup>
    <ErrorBoundaryGroup.Reset trigger={({ resetGroup }) => <button onClick={resetGroup}>Reset All</button>} />
    <ErrorBoundary
      renderFallback={({ reset, error }) => <button onClick={reset}>reset {JSON.stringify(error)}</button>}
    >
      <ThrowComponent />
    </ErrorBoundary>
    <ErrorBoundary
      renderFallback={({ reset, error }) => <button onClick={reset}>reset {JSON.stringify(error)}</button>}
    >
      <ThrowComponent />
    </ErrorBoundary>
  </ErrorBoundaryGroup>
);

Edge cases

If No ErrorBoundaryGroup as parent

but, default ErrorBoundary without ErrorBoundaryGroup also work self

const Example = () => (
    <ErrorBoundary
      renderFallback={({ reset, error }) => <button onClick={reset}>reset {JSON.stringify(error)}</button>}
    >
      <ThrowComponent />
    </ErrorBoundary>)

Multiple nested ErrorBoundaryGroup

If reset most top level parent, Every ErrorBoundaryGroup will be reset.

const Example = () => (
  <ErrorBoundaryGroup>
    <ErrorBoundaryGroup.Reset trigger={({ resetGroup }) => <button onClick={resetGroup}>Reset All</button>} />
	<ErrorBoundaryGroup>
      <ErrorBoundaryGroup.Reset trigger={({ resetGroup }) => <button onClick={resetGroup}>Reset only inside nested ErrorBoundaryGroup</button>} />
    <ErrorBoundary
      renderFallback={({ reset, error }) => <button onClick={reset}>reset {JSON.stringify(error)}</button>}
    >
      <ThrowComponent />
    </ErrorBoundary>
    </ErrorBoundaryGroup>
    <ErrorBoundary
      renderFallback={({ reset, error }) => <button onClick={reset}>reset {JSON.stringify(error)}</button>}
    >
      <ThrowComponent />
    </ErrorBoundary>
    <ErrorBoundary
      renderFallback={({ reset, error }) => <button onClick={reset}>reset {JSON.stringify(error)}</button>}
    >
      <ThrowComponent />
    </ErrorBoundary>
  </ErrorBoundaryGroup>
);

but If turn on blockOutside prop of ErrorBoundaryGroup, It will block parent's resetting to children of itself

const Example = () => (
  <ErrorBoundaryGroup>
    <ErrorBoundaryGroup.Reset trigger={({ resetGroup }) => <button onClick={resetGroup}>Reset All</button>} />
	<ErrorBoundaryGroup blockOutside>{/*use blockOutside to block resetting by outside of ErrorBoundaryGroup.Reset*/}
      <ErrorBoundaryGroup.Reset trigger={({ resetGroup }) => <button onClick={resetGroup}>Reset only inside nested ErrorBoundaryGroup</button>} />
    <ErrorBoundary
      renderFallback={({ reset, error }) => <button onClick={reset}>reset {JSON.stringify(error)}</button>}
    >
      <ThrowComponent />
    </ErrorBoundary>
    </ErrorBoundaryGroup>
    <ErrorBoundary
      renderFallback={({ reset, error }) => <button onClick={reset}>reset {JSON.stringify(error)}</button>}
    >
      <ThrowComponent />
    </ErrorBoundary>
    <ErrorBoundary
      renderFallback={({ reset, error }) => <button onClick={reset}>reset {JSON.stringify(error)}</button>}
    >
      <ThrowComponent />
    </ErrorBoundary>
  </ErrorBoundaryGroup>
);

Comment Reply

Thanks for your pull request, @manudeli. Now I see the problem. However I see some problems as below:

I think ErrorBoundary.ResetKey and withResetKey exposes implementation detail to its public API. I think we should only expose its intended behavior to the public API.

What we expect for withResetKey is to define a shared boundary reset context. so in this case withErrorBoundaryContext or withErrorBoundaryGroup might be a better naming.

Cool, I renamed ResetKey into ErrorBoundaryGroup after your comment.

Why should ErrorBoundary.ResetKey and BaseErrorBoundary be separated? Why not merging them into one, i.e. ErrorBoundary? (What about defining a default reset key and making the default ErrorBoundary use it in case it has no providers? It wouldn't break the existing code and will make our codebase easier to maintain)

I worried that changing original implementation can be rude for you. but If you accept adding new feature of original ErrorBoundary, why not. so I add new feature of ErrorBoundary can be reset by ErrorBoundaryGroup's groupResetKey, If there is no ErrorBoundaryGroup as parent, it won't work.

and I removed ErrorBoundary.ResetKey and BaseErrorBoundary 👍

I think this is a new feature introduced in slash libraries, and we think new features should have a test case. Can you add a test case in this pull request? If not, we could add some test cases in this pull request.

After your checking this implementation and accept all interfaces (welcome your changing too), I will try to add test code.
Tell me if you accept all interfaces.

Actually I have very small experience to add test code. but I want to try it. Allow your patience to wait me please :)

manudeli · 2022-11-28T12:37:37Z

packages/react/async-boundary/src/AsyncBoundary.tsx

-interface ResetRef {
-  reset?(): void;
-}


I delete unnecessary ResetRef. I think this interface need to be driven from ErrorBoundary.

manudeli · 2022-11-28T12:38:34Z

packages/react/error-boundary/src/ErrorBoundary.tsx

+const ErrorBoundary = forwardRef<{ reset?(): void }, ComponentProps<typeof BaseErrorBoundary>>((props, resetRef) => {
+  const { groupResetKey } = useErrorBoundaryGroup();
+  const resetKeys = groupResetKey ? [groupResetKey, ...(props.resetKeys || [])] : props.resetKeys;
+
+  const ref = useRef<BaseErrorBoundary | null>(null);
+  useImperativeHandle(resetRef, () => ({
+    reset: () => ref.current?.resetErrorBoundary(),
+  }));
+
+  return <BaseErrorBoundary {...props} resetKeys={resetKeys} />;
+});


I added new feature using ErrorBoundaryGroup's groupResetKey.

manudeli · 2022-11-28T12:39:40Z

packages/react/error-boundary/src/ErrorBoundaryGroup.tsx

+const ErrorBoundaryGroupReset = ({ trigger }: { trigger: ComponentType<{ resetGroup: () => void }> }) => {
+  const { resetGroup } = useErrorBoundaryGroup();
+  const Trigger = trigger;
+
+  return <Trigger resetGroup={resetGroup} />;
+};


I hided groupResetKey. because default ErrorBoundary can use it defaultly now.

raon0211 · 2022-11-29T17:06:18Z

So cool, take your time on tests. Let me know when tests are done. (If it is too hard, don't hesitate to ask us!)

…st.tsx when developing

manudeli · 2022-11-30T23:29:07Z

So cool, take your time on tests. Let me know when tests are done. (If it is too hard, don't hesitate to ask us!)

@raon0211
Could you add test code for this PR please? I tried to add test code. but because of my lack experience, I couldn't get confidence for my test code can prove this PR is believable. I thought I need to learn how to write test code first. so I need your help.

by the way, During trying to add test code, I encounter this issue

before trying adding test code, I solved this problem in only this package(@toss/error-boundary) in this commit, but it's just my way and this can make difference with build convention of @toss/slash's all packages.

If adding test code is not scope of this PR, I'll delete it. check it please.

raon0211 · 2022-12-01T06:26:24Z

Okay, please let us some time. We plan to add one in this week.

raon0211 · 2022-12-05T16:40:49Z

I added some tests here
86094ef, and actually found a bug! (The reset function was not being called)

raon0211 · 2022-12-05T16:42:46Z

packages/react/error-boundary/src/ErrorBoundary.tsx

@@ -95,3 +106,17 @@ export default class ErrorBoundary extends Component<PropsWithRef<PropsWithChild
    return children;
  }
 }
+
+const ErrorBoundary = forwardRef<{ reset?(): void }, ComponentProps<typeof BaseErrorBoundary>>((props, resetRef) => {
+  const { groupResetKey } = useErrorBoundaryGroup();


Would exposing groupResetKey be necessary? While writing some tests, I thought only exposing the reset function might be nice.

const reset = useResetErrorBoundaryGroup(); // or... const group = useErrorBoundaryGroup(); group.reset(); // The implementation detail `groupResetKey` is private, it only exposes `reset`

Thanks👍 I removed groupResetKey and renamed resetGroup as reset like your comment in df96812

raon0211 · 2022-12-05T16:43:43Z

packages/react/error-boundary/src/ErrorBoundaryGroup.tsx

+    </ErrorBoundaryGroupContext.Provider>
+  );
+};
+ErrorBoundaryGroup.Reset = ErrorBoundaryGroupReset;


What would be the use case for ErrorBoundaryGroup.Reset? Wouldn't exposing useErrorBoundaryGroup sufficient? 👀

I thought developer that have to use class components may want this ErrorBoundaryGroup.Reset.

also, HOC using useErrorBoundaryGroup hook can be another option to do it. but I thought providing ErrorBoundaryGroup.Reset can be easiest way to reset ErrorBoundaryGroup.

If we provide ErrorBoundaryGroup.Reset, developer don't have to import useErrorBoundaryGroup. because ErrorBoundaryGroup.Reset is compounded for it.

I want to contain meaning of resetting group into ErrorBoundaryGroup more definitely by using compound component pattern

import { ErrorBoundaryGroup } from '@toss/error-boundary' // If use ErrorBoundaryGroup.Reset, developer don't have to import another thing to reset. <ErrorBoundaryGroup> <ErrorBoundaryGroup.Reset trigger={({ reset }) => <ClassComponent onClick={reset} />}> <ErrorBoundary> <Children /> </ErrorBoundary> </ErrorBoundaryGroup>

but, I think it's up to you. both implementation are good for me! choose it freely please.

If implementing the component is easy and straightforward, I prefer to provide smaller sets of API, since it would improve our maintainability. Users of our library could also benefit in increased flexibility (they could decide how to design ErrorBoundary.Reset's API.) What do you think?

I understood your intention. I removed ErrorBoundary.Reset in b12ec9c

raon0211 · 2022-12-05T16:44:39Z

Sorry for adding the tests late. It was a hard time for me last week, and I was so busy. Thanks for being patient 🙇

…y, reset

manudeli · 2022-12-06T07:26:59Z

I added some tests here 86094ef, and actually found a bug! (The reset function was not being called)

Oh,, I mistook it (The reset function was not being called). Thanks for your eagle eyes 👀 and fixing

raon0211

This is a great work. Thanks, @manudeli !

feat(error-boundary): add resetKey

269353e

manudeli force-pushed the feat/error-boundary/resetKey branch from 3d6cc3e to 269353e Compare November 24, 2022 20:01

manudeli mentioned this pull request Nov 24, 2022

[Feature]: How about change AsyncBoundaryProvider(@toss/async-boundary) to ResetErrorBoundaryProvider(@toss/error-boundary)? #144

Closed

1 task

feat(ResetKey): add withResetKey.Provider/Consumer

ba03ae8

manudeli force-pushed the feat/error-boundary/resetKey branch from 9ee0a70 to ba03ae8 Compare November 26, 2022 10:42

fix(react-boundary): edit ResetKey to compound component like withRes…

47a42f3

…etKey

raon0211 reviewed Nov 28, 2022

View reviewed changes

fix(error-boundary): add ErrorBoundaryGroup

130c24e

manudeli commented Nov 28, 2022

View reviewed changes

manudeli changed the title ~~feat(error-boundary): add resetKey~~ feat(error-boundary): add ErrorBoundaryGroup Nov 28, 2022

manudeli requested a review from raon0211 November 28, 2022 15:06

refactor(error-boundary): remove unnecessary prop

3c6c756

manudeli added 2 commits November 30, 2022 18:42

Merge branch 'main' into feat/error-boundary/resetKey

e73df8e

fix(error-boundary): add tsconfig.build.json to avoid excluding **.te…

fa15a42

…st.tsx when developing

manudeli mentioned this pull request Nov 30, 2022

[BUG]: Current, Many packages after build can contain unnecessary **.test.d.ts. #160

Closed

Merge branch 'main' into feat/error-boundary/resetKey

96e7a17

manudeli and others added 2 commits December 1, 2022 23:09

Merge branch 'main' into feat/error-boundary/resetKey

6067c0d

test: Add error boundary tests

86094ef

raon0211 reviewed Dec 5, 2022

View reviewed changes

remove

5a29782

manudeli force-pushed the feat/error-boundary/resetKey branch from 396acb7 to 9a15657 Compare December 6, 2022 03:54

refactor(error-boundary): rename groupResetKey, resetGroup to resetKe…

df96812

…y, reset

manudeli force-pushed the feat/error-boundary/resetKey branch from 9a15657 to df96812 Compare December 6, 2022 04:01

fix(error-boundary): remove ErrorBoundary.Reset

b12ec9c

raon0211 approved these changes Dec 6, 2022

View reviewed changes

raon0211 merged commit 34204c1 into toss:main Dec 6, 2022

This was referenced Jul 6, 2023

chore(@toss/error-boundary): change useIsMounted dependencies local to @toss/react #277

Merged

fix(error-boundary): remove ErrorBoundaryGroup's not on purpose reset because of isMounted's side effect #287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(error-boundary): add ErrorBoundaryGroup #157

feat(error-boundary): add ErrorBoundaryGroup #157

manudeli commented Nov 24, 2022 •

edited

Loading

netlify bot commented Nov 24, 2022 •

edited

Loading

raon0211 commented Nov 28, 2022

raon0211 Nov 28, 2022

raon0211 Nov 28, 2022

manudeli commented Nov 28, 2022 •

edited

Loading

manudeli Nov 28, 2022

manudeli Nov 28, 2022

manudeli Nov 28, 2022

raon0211 Dec 5, 2022

raon0211 commented Nov 29, 2022 •

edited

Loading

manudeli commented Nov 30, 2022 •

edited

Loading

raon0211 commented Dec 1, 2022

raon0211 commented Dec 5, 2022

raon0211 Dec 5, 2022 •

edited

Loading

manudeli Dec 6, 2022 •

edited

Loading

raon0211 Dec 5, 2022

manudeli Dec 6, 2022 •

edited

Loading

raon0211 Dec 6, 2022

manudeli Dec 6, 2022

raon0211 commented Dec 5, 2022

manudeli commented Dec 6, 2022

raon0211 left a comment

feat(error-boundary): add ErrorBoundaryGroup #157

feat(error-boundary): add ErrorBoundaryGroup #157

Conversation

manudeli commented Nov 24, 2022 • edited Loading

Overview

Referencing Issue

Problem

Solution

How to use

Using withResetKey

Nested Component using useResetKey

Using ResetKey.Provider, ResetKey.Consumer

or Using ResetKey.Provider, ErrorBoundary.ResetKey

PR Checklist

netlify bot commented Nov 24, 2022 • edited Loading

👷 Deploy request for slash-libraries pending review.

raon0211 commented Nov 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manudeli commented Nov 28, 2022 • edited Loading

ErrorBoundaryGroup

If ErrorBoundaryGroup as parent of ErrorBoundary

Edge cases

If No ErrorBoundaryGroup as parent

Multiple nested ErrorBoundaryGroup

Comment Reply

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raon0211 commented Nov 29, 2022 • edited Loading

manudeli commented Nov 30, 2022 • edited Loading

raon0211 commented Dec 1, 2022

raon0211 commented Dec 5, 2022

raon0211 Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

manudeli Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manudeli Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raon0211 commented Dec 5, 2022

manudeli commented Dec 6, 2022

raon0211 left a comment

Choose a reason for hiding this comment

manudeli commented Nov 24, 2022 •

edited

Loading

netlify bot commented Nov 24, 2022 •

edited

Loading

manudeli commented Nov 28, 2022 •

edited

Loading

raon0211 commented Nov 29, 2022 •

edited

Loading

manudeli commented Nov 30, 2022 •

edited

Loading

raon0211 Dec 5, 2022 •

edited

Loading

manudeli Dec 6, 2022 •

edited

Loading

manudeli Dec 6, 2022 •

edited

Loading