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

Editor not displaying at first load with Strictmode #338

Closed
EmrickPesce opened this issue Oct 25, 2022 · 6 comments · Fixed by #346
Closed

Editor not displaying at first load with Strictmode #338

EmrickPesce opened this issue Oct 25, 2022 · 6 comments · Fixed by #346
Labels
squad:platform Issue to be handled by the Platform team.
Milestone

Comments

@EmrickPesce
Copy link

EmrickPesce commented Oct 25, 2022

Hello team !

Thank you for your hard work :)

I noticed that my editor wasn't displayed when I start my server (with npm start). To make it showing, I need to reload the page on my navigator, and then it's working fine.

After multiple hours of research I found out that it was occurring because of the React StrictMode. This also comes because I retrieve some data from my own server, and I use useEffect for waiting that all data is loaded before displaying anything.

You can reproduce it in a new React App, with just this code:

// App.js:
import React, { useEffect, useState } from "react";
import ClassicEditor from "@ckeditor/ckeditor5-build-classic";
import { CKEditor } from "@ckeditor/ckeditor5-react";

function App() {
  const [isLoading, setIsLoading] = useState(true);
  var someData = "notEmpty";

  useEffect(() => {
    setTimeout(function () {
      someData && setIsLoading(false);
    }, 1000); // Simulate some data loading from a server
  }, [someData]);

  return (
    <>
      {isLoading ? (
        <i>Logo loading</i>
      ) : (
        <>
          <div className="App">
            <CKEditor
              editor={ClassicEditor}
              data="<p>Hello from CKEditor 5!</p>"
            />
          </div>
        </>
      )}
    </>
  );
}

export default App;
// index.js:
`import React from 'react';
import ReactDOM from 'react-dom/client';
import './index.css';
import App from './App';
import reportWebVitals from './reportWebVitals';

const root = ReactDOM.createRoot(document.getElementById('root'));
root.render(
  <React.StrictMode>
    <App />
  </React.StrictMode>
);

// If you want to start measuring performance in your app, pass a function
// to log results (for example: reportWebVitals(console.log))
// or send to an analytics endpoint. Learn more: https://bit.ly/CRA-vitals
reportWebVitals();

Hope you can reproduce it. If you have any questions, do not hesitate;

Thank you for all !

@pomek pomek added the squad:platform Issue to be handled by the Platform team. label Oct 25, 2022
@corymharper
Copy link
Contributor

corymharper commented Oct 28, 2022

I stuck this in a sandbox for convenience: https://codesandbox.io/s/ecstatic-lamarr-85bjw8?file=/src/index.js

You can reproduce the issue by opening the sandbox website in a new tab, any initial load will display the error, but for some reason refreshes won't. The fact that its happening only in strict mode and didn't happen before React 18 probably means it has something to do with the new double invocation behavior described here:

To help surface these issues, React 18 introduces a new development-only check to Strict Mode. This new check will automatically unmount and remount every component, whenever a component mounts for the first time, restoring the previous state on the second mount.

it also means it does not happen in production, more than likely.

@corymharper
Copy link
Contributor

Some more information I've discovered:

On the renders the the editor does not initialize, a new componentDidMount lifecycle method is being called before the completion of componentWillUnmount from the previous cycle has been run to completion. Specifically _destroyEditor is called, and starts to run, but when the call to this.watchdog.destroy occurs in that method, a new call to componentDidMount has already been registered. It calls _initializeEditor, however the line in _destroyEditor that unsets the watchdog has not been run yet at this point (this.watchdog = null). Such that when this check occurs, there is a value assigned to it, and no new editor is initialized. Afterwards the function that destroys the editor runs to completion and destroys the editor with no initialization afterwards.

@corymharper
Copy link
Contributor

When the component fails to render properly, this is what the lifecycle looks like (confirmed via logging):

> Component mounted
> Editor created
> Component will unmount
> Watchdog destruction begins
> Component mounted
> Watchdog already present, no new watchdog or editor initialized
> Watchdog destruction completed
> Watchdog unset after destroy

As you can see from those logs, the editor finishes being created, an unmount happens, the process of destroying the watchdog and later the editor begins, componentDidMount fires with _initializeEditor, but this.watchdog is still present, so it skips initializing another watchdog/editor. Subsequently, the destruction process is completed, so you are left with no watchdog/editor.

@corymharper
Copy link
Contributor

corymharper commented Nov 9, 2022

I am currently fixing this locally by forcing the _initializeEditor to wait for any pending destruction. Here's a summary of my implementation:

// In the constructor:
export default class CKEditor extends React.Component {
	constructor( props ) {
		super( props );

		this.destructionInProgress = null;
...
// _initializeEditor():
	async _initializeEditor() {
		if ( this.destructionInProgress ) {
			await this.destructionInProgress;
		}
// componentWillUnmount():
	async componentWillUnmount() {
		this.destructionInProgress = new Promise( resolve => {
			this._destroyEditor().then( () => {
				resolve();
			} );
		} ).then( () => {
			this.destructionInProgress = null;
		} );
	}

If these changes seem appropriate, I'll make a pull request with them! If the maintainers have any comments or suggestions though, I'm completely open to changing the implementation and still submitting a pull request.

@pomek
Copy link
Member

pomek commented Nov 10, 2022

Hi @corymharper, thanks for the detailed comments you have written. They explain the problem a bit. We'd appreciate it if you could prepare a PR with these changes.

@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Nov 10, 2022
@corymharper
Copy link
Contributor

@pomek at your convenience, #342 is ready for review.

pomek added a commit that referenced this issue Nov 23, 2022
Fix: Fixed the component initialization procedure to enforce cleanup completion before subsequent editor initialization. Closes #321. Closes #338.

Thanks to @corymharper.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Nov 23, 2022
@CKEditorBot CKEditorBot added this to the iteration 59 milestone Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment