Skip to content

Firefly-1127: upgrade react to 18#1396

Merged
robyww merged 1 commit intodevfrom
FIREFLY-1127-react-18
Jun 23, 2023
Merged

Firefly-1127: upgrade react to 18#1396
robyww merged 1 commit intodevfrom
FIREFLY-1127-react-18

Conversation

@robyww
Copy link
Copy Markdown
Contributor

@robyww robyww commented Jun 16, 2023

Firefly-1127: upgrade react to 18

  • so trying out useTransition() hook in useStoreConnector
  • Trying useDeferredValue in ImageViewer

building.

  • before building
    • cd <path-to-firefly>/firefly
    • rm -rf node_modules
    • yarn --frozen-lockfile
  • then normal gradle build

Test:

Important notes about React 18:

  • No warning about setState on unmounted components
    • This is ready good news! The warning never made much sense.
    • We could do a lot of clean up in our hooks
  • Components can now return undefined
    • No action necessary but we should stay aware of this. It will be an convenience
  • useTransition / startTransition (bad naming, I think) are ways to make a component render run at a lower priority, It will not block normal renders
  • useDeferredValue is another ways to make component run at a lesser priority. I am not sure I quite understand it yet but I put I am trying it in one compoenent

@robyww robyww self-assigned this Jun 21, 2023
@robyww robyww added this to the 2023.2 milestone Jun 21, 2023
@robyww robyww force-pushed the FIREFLY-1127-react-18 branch from 1875480 to 71c4447 Compare June 21, 2023 16:07
@robyww robyww marked this pull request as ready for review June 21, 2023 16:07
Copy link
Copy Markdown
Contributor

@loitly loitly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/firefly/js/Firefly.js
Comment on lines +379 to +381
const rootToUse = root ?? createRoot(e);
const webApi= isUsingWebApi(webApiCommands);
const doAppRender= () => ReactDOM.render(React.createElement(viewer, {...props, normalInit: !webApi }), e);
const doAppRender= () => rootToUse.render( React.createElement(viewer, {...props, normalInit: !webApi }) );
Copy link
Copy Markdown
Contributor

@loitly loitly Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried adding an ErrorBoundary around the 'viewer' to see if it'll catch all uncaught errors?
It may not work as expected, but it's worth a try. There's very little code needed to make this happen.

const app = React.createElement(viewer,   ...
rootToUse.render(
  <ErrorBoundary>
    {app}
  </ErrorBoundary>,
  
  ...  

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking for ErrorBoundary but could not find it in the docs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still can't fine it. Can you point me to the docs?

Copy link
Copy Markdown
Contributor Author

@robyww robyww Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell this is only documented in the legacy API my creating such a component using componentDidCatch() and getDerivedStateFromError(), but this is in what they now call their Legacy React API.

Copy link
Copy Markdown
Contributor

@loitly loitly Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea whether or not an uncaught exception would propagate to the root rendering. I was only curious to see if you've tried. Below is a 'class' based implementation. There's a functional version as well. If it does, then we can at least intercept it and provide messages with some sort of reset link.

https://legacy.reactjs.org/docs/error-boundaries.html

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

  static getDerivedStateFromError(error) {
    // Update state so the next render will show the fallback UI.
    return { hasError: true };
  }

  componentDidCatch(error, errorInfo) {
    // You can also log the error to an error reporting service
    logErrorToMyService(error, errorInfo);
  }

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

    return this.props.children; 
  }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is what I have found as well. This was added in react 16. They are now acting like it is 'legacy'. Either way it is out of scope for this PR. We could have put this in for the last 2 years. I think it is a different task from the upgrade.

@robyww
Copy link
Copy Markdown
Contributor Author

robyww commented Jun 22, 2023

I think there is a problem with loading react in the API. I will look into it

@robyww robyww force-pushed the FIREFLY-1127-react-18 branch from 71c4447 to d1bca1a Compare June 22, 2023 22:53
Copy link
Copy Markdown
Contributor

@loitly loitly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug fixed. Good to go.

  - upgrading some other packages as well
  - Trying out useTransition() hook in useStoreConnector
  - Trying useDeferredValue in ImageViewer
@robyww robyww force-pushed the FIREFLY-1127-react-18 branch from d1bca1a to 2d725fe Compare June 23, 2023 17:14
@robyww robyww merged commit 2b54439 into dev Jun 23, 2023
@robyww robyww deleted the FIREFLY-1127-react-18 branch June 23, 2023 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants