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

Audit of React usage #5479

Merged
merged 21 commits into from Dec 29, 2018
Merged

Audit of React usage #5479

merged 21 commits into from Dec 29, 2018

Conversation

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Oct 11, 2018

I went through some of the places where we use React, and tried to clean up some of the code to be more idiomatic, correct as far as lifecycle issues go, etc.

I'm turning to the toolbar buttons next.

CC @gnestor, @ian-r-rose

Closes #5766

jasongrout added 6 commits Oct 11, 2018
We weren’t really using react philosophy, just using it to render the initial DOM, and this meant we had to do a lot of workarounds to retrieve DOM elements. I think it’s simpler to just convert this back to traditional DOM manipulation unless/until we want to move everything (events, etc.) over to React.
packages/json-extension/package.json
This takes care of bridging the basic lifecycle methods between Phosphor and React.
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 11, 2018

As a test for this code, I experimented with the save button enabling (see #5475 and #5454). @dharmaquark, what do you think of 0185a7d?

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Oct 11, 2018

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 11, 2018

Sure. Here's the code: https://github.com/jupyterlab/jupyterlab/pull/5479/files#diff-94b1d4a8aa4bd27916f62d1876907000R19

The main difference is that it calls the react unmount function when phosphor is about to detach the DOM node, to give the react component a chance to dispose itself.

I also factored out the actual render-to-DOM function, and made it return a promise so that you could call it directly and get a promise back when the rendering was done. I did this refactoring to support the rendermime cases, though in the end I didn't use this class there after all. I left this refactoring in since it's still potentially useful to know when a rendering is finished.

Other large parts of this PR are making our use of React more consistent.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 11, 2018

I also deleted the ReactElementWidget class, since it encouraged anti-patterns in inheritance (subclass would have a completely different constructor signature than superclasses). The ReactElementWidget class was only saving about 5 lines of code, and I thought it would be clearer and more explicit anyway to use ReactWidget by itself.

One more thing to flag: I noticed (almost?) no one was using the vdom renderer class/pattern (i.e., having a vdom model triggering updates). Nearly everywhere people were just using react directly, probably since it's much lighter weight. So I refactored out the relevant parts of the vdom renderer to the new ReactWidget (e.g., the idea of a phosphor update rerendering the react component, the mounting/unmounting lifecycle events, etc.), and now the vdom renderer class just adds the model pattern on top of ReactWidget, if people would like to use that pattern.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 11, 2018

@dharmaquark - I think there is still some work to do to sort out the difference between the contentsModel writable state, and the model readOnly state. That was perceptive for you to observe that we were using the model readOnly state in that save handler to decide about saving.

As far as I can tell, the model readOnly state is never set to true? It certainly isn't set to true for non-writable contentsModels. I think there is something more to do about those two attributes and how they interact with each other.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 11, 2018

I also deleted the ReactElementWidget class

Actually, I left the actual class in for now, since it's called in the dialog code, even though I couldn't find anywhere that codepath is actually used.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 11, 2018

even though I couldn't find anywhere that codepath is actually used.

Found where react is used in the dialog is used - the help extension.

Looking at the codebase, it seems we are using React in two basic contexts:

A. When we have simple html that is being updated in response to model changes,
B. As a templating language for rendering html a single time, where the html won't change (this is more JSX than react, but does use react to do a single render).

I was thinking that react was appropriate in situation A, but am coming around to using react in situation B as well.

@vidartf
Copy link
Member

@vidartf vidartf commented Oct 11, 2018

I noticed (almost?) no one was using the vdom renderer class/pattern (i.e., having a vdom model triggering updates).

I have used this before, but that was before there were any alternatives. In general, I felt that it wasn't clear from the documentation what the advantages of using this pattern was. Is it simply an adapter to help mesh the two frameworks? Is it performant? Is it best practices? For which cases is it recommended, for which cases not?

There might have been new documentation added since I looked at it though, so it might have been solved partially.

dharmaquark
Copy link
Contributor

dharmaquark commented on 0185a7d Oct 11, 2018

This looks great! Do you think it would be helpful to update the tooltip of the button when disabled, with text similar to the dialog, to give an indication that the document is ‘read only’?

dharmaquark
Copy link
Contributor

dharmaquark commented on 0185a7d Oct 11, 2018

That might be tricky since there’s no signal or callback when the enabled property changes, maybe something worth considering down the line, depending on feedback we get from users? Could be helpful for other buttons whose enabled logic is less intuitive than save’s

Copy link
Contributor

@ellisonbg ellisonbg left a comment

Left a few comments and questions...thanks for working on this :-)

packages/apputils/src/vdom.ts Show resolved Hide resolved
packages/apputils/src/vdom.ts Show resolved Hide resolved
@@ -109,7 +134,7 @@ export abstract class VDomRenderer<
*
* All messages will re-render the element.
*/
export class ReactElementWidget extends VDomRenderer<any> {
export class ReactElementWidget extends ReactWidget {
Copy link
Contributor

@ellisonbg ellisonbg Oct 11, 2018

Is this the class we would typically use? One of the things I found when I implemented the toolbar buttons was that it was very nice to have a Widget subclass that took the props in its constructor. Then the widgets are as easy to use as the underlying react components. If we want to advocate for that pattern, maybe the right answer is to not offer this class, but instead recommend that people directly subclass ReactWidget and override the constructor (to take props) and render methods.

Copy link
Contributor

@ellisonbg ellisonbg Oct 11, 2018

We would also provide a generic subclass that has a constructor that takes the more generic react props type.

Copy link
Contributor Author

@jasongrout jasongrout Oct 11, 2018

In the case where we are using react as a templating language, the pattern of doing ReactElementWidget(<some react component>) is kind of nice to generate a simple phosphor widget with the requested html.

Copy link
Contributor

@gnestor gnestor Dec 21, 2018

I agree with @jasongrout that ReactElementWidget could be a function vs. class:

const KernelName = ReactElementWidget(<KernelNameComponent {...props} />);

This would clean up the code even more. This doesn't allow KernelName to override ReactWidget but it looks like the only time that is done is to set it's node's class name. So maybe ReactElementWidget signature should be ReactElementWidget(element: React.ReactElement, className: string:

const KernelName = ReactElementWidget(<KernelNameComponent {...props} />, 'jp-KernelName');

Copy link
Member

@saulshanabrook saulshanabrook Dec 26, 2018

I changed this to a function. It is now ReactWidget.create:

const KernelName = ReactWidget.create(<KernelNameComponent {...props} />);

or

const KernelName = ReactWidget.create(<KernelNameComponent {...props} />);
KernelName.addClass('jp-KernelName')

Copy link
Contributor Author

@jasongrout jasongrout Dec 28, 2018

@saulshanabrook - I like it!


let host = this.editorHostNode;
this.headerNode = document.createElement('div');
Copy link
Contributor

@ellisonbg ellisonbg Oct 11, 2018

I am fine with the move away from react in this, case, but for utility we may also want to have a react component that wraps codemirror and our code editor stuff, so it can be used in other places where extension authors are using react.

Copy link
Contributor

@ellisonbg ellisonbg Oct 11, 2018

Although, I am wondering if actually using react here does make sense. Is the logic to not have react used in this part of the DOM/codebase?

Copy link
Contributor Author

@jasongrout jasongrout Dec 13, 2018

The logic in this class was pretty tricky. It seemed that the idea was to use react as a template language, not really to have react re-render things, or really to have a complete react component. The problem here with using react as a template engine is that the other code did a lot of stuff with individual DOM elements that were generated, so it was a bit awkward getting at those DOM elements using class selectors. I think it's easier to just directly create the elements and structure, and directly point to the elements as we create them.

(This was an old comment...)

Copy link
Member

@saulshanabrook saulshanabrook Dec 26, 2018

To keep react here and clean up the dom handling, we could use refs to return the dom nodes of different parts, without resorting to all the getElementsByClassName calls.

However, my preference would be to remove access to the internal nodes from the API entirely. I can't find anywhere in our own codebase where this is used (but I could have missed them).

Could someone point me to a place where these are needed?

Copy link
Contributor Author

@jasongrout jasongrout Dec 28, 2018

Could someone point me to a place where these are needed?

References to these nodes are needed in this specific class, but I don't think they need to be public attributes.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 12, 2018

I think this topic is broad enough in scope that we should discuss it at the JupyterLab team meeting next month. @sccolbert had created a RenderWidget that had stalled due to lack of consensus about whether to use a model: https://github.com/phosphorjs/phosphor/pull/299/files.
We may also want to change IRenderMime so that the interface matches that of our standard react renderer, with different options/props given.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Oct 15, 2018

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 15, 2018

I meant the in person meeting next month. 😀

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Oct 15, 2018

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Nov 2, 2018

I happen to have an unpushed change on my local branch for this. I'm not sure if it's still needed, so I'll just log it here in case it helps:

diff --git a/packages/apputils/src/dialog.ts b/packages/apputils/src/dialog.ts
index 87e7740b3..365150f09 100644
--- a/packages/apputils/src/dialog.ts
+++ b/packages/apputils/src/dialog.ts
@@ -438,7 +438,7 @@ export namespace Dialog {
   /**
    * The header input types.
    */
-  export type HeaderType = React.ReactElement<any> | string;
+  export type HeaderType = string;
 
   /**
    * The result of a dialog.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Nov 27, 2018

I just opened #5689 to try another approach.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Nov 28, 2018

Current approach:

class List<M> extends React.Component<SessionProps<M>, { models: M[] }> {
  constructor(props: SessionProps<M>) {
    super(props);
    this.state = { models: toArray(props.manager.running()) };
  }
  render() {
    return (
      <ul>
        {this.state.models.map((m, i) => (
          <Item key={i} model={m} {...this.props} />
        ))}
      </ul>
    );
  }
  componentDidMount() {
    if (this.props.available) {
        this.setState({
          models: models.filter(this.props.filterRunning || (_ => true))
        })
    }
  }
}

With render props

function List<M>(props: SessionProps<M>) {
  function slot(_: any, models: M[]) {
    return (
      <ul className={LIST_CLASS}>
        {models.map((m, i) => (
          <Item key={i} model={m} {...props} />
        ))}
      </ul>
    );
  }

  return (
    <UseSignal
      signal={props.manager.runningChanged}
      initial={[null, toArray(props.manager.running())]}
      slot={slot}
    />
  );
}

With react hooks (ready in Q1 2019):

function List<M>(props) {
  const [models, setModels] = useState(() => toArray(props.manager.running()));

  function handleRunningChanged(_, models) {
    setModels(models.filter(props.filterRunning));
  }

  useEffect(() => {
    props.manager.runningChanged.connect(handleRunningChanged);
    return function cleanup() {
      props.manager.runningChanged.disconnect(handleRunningChanged);
    };
  });

  return (
    <ul>
      {models.map((m, i) => (
        <Item key={i} model={m} {...this.props} />
      ))}
    </ul>
  );
}

With custom react hook (ready in Q1 2019):

function List<M>(props) {
  const [models, setModels] = useSignal(
    () => toArray(props.manager.running()),
    props.manager.runningChanged,
    (sender, args) => args.filter(props.filterRunning)
  );

  

  return (
    <ul>
      {models.map((m, i) => (
        <Item key={i} model={m} {...this.props} />
      ))}
    </ul>
  );
}

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Nov 29, 2018

A small simplification to the custom hook example:

function List<M>(props) {
  const [models,] = useSignal(
    () => toArray(props.manager.running()),
    props.manager.runningChanged,
    (sender, args) => args.filter(props.filterRunning)
  );

  return (
    <ul>
      {models.map((m, i) => (
        <Item key={i} model={m} {...this.props} />
      ))}
    </ul>
  );
}

@gnestor
Copy link
Contributor

@gnestor gnestor commented Nov 29, 2018

Here's an example of what it looks like to provide the render prop as children vs. slot. This is how render props are normally implemented. It's more simple and more odd at the same time (e.g. nesting a function in JSX vs. element):

<WithSignal> definition:

class WithSignal<T> extends React.Component<
  { initialValue: T, signal: ISignal<T>, children: (arg: T) => React.ReactNode },
  { value: T }
> {
  state = { value: null };

  componentDidMount() {
    this.props.signal.connect((sender, args) => {
      this.setState({ value: args });
    });
  }

  render() {
    return this.props.children(
      this.state.value ? this.state.value : this.props.initialValue
    );
  }
}

<WithSignal> implementation:

<WithSignal
  initialValue={toArray(manager.running())}
  signal={manager.runningChanged}
>
  {value => <span>{value.title}</span>}
</WithSignal>

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Dec 17, 2018

@saulshanabrook could you ping me when this is ready for final review? Thanks!

@saulshanabrook saulshanabrook changed the title [WIP] Audit of React usage Audit of React usage Dec 19, 2018
packages/apputils/src/toolbar.tsx Outdated Show resolved Hide resolved
packages/apputils/src/toolbar.tsx Outdated Show resolved Hide resolved
packages/apputils/src/vdom.ts Show resolved Hide resolved
packages/apputils/src/vdom.ts Outdated Show resolved Hide resolved
@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Dec 21, 2018

I just went over this myself and requested some changes from myself. I will comment again when I have resolved them.

packages/apputils/src/vdom.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@gnestor gnestor left a comment

This is looking good to me. I think that once @saulshanabrook's comments are resolved, this will be ready to merge

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Dec 26, 2018

I have gone over this again and I believe we have a couple more things to settle on before this is merged:

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 29, 2018

Thanks! With those changes, I think this is good from my perspective if tests pass.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 29, 2018

The test failures seem completely unrelated, and a bit spurious. For example, on appveyor:

  @jupyterlab/notebook
    NotebookActions
      #runAll()
        × should stop executing code cells on an error
          HeadlessChrome 0.0.0 (Windows 8.1 0.0.0)
        Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 29, 2018

I'll merge this, and we can iterate on that issue about the JSON editor having public attributes for its DOM elements.

@jasongrout jasongrout merged commit 3044387 into jupyterlab:master Dec 29, 2018
1 of 3 checks passed
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 29, 2018

Thanks again @saulshanabrook for pushing this one through to the end!

@jasongrout jasongrout added this to the 1.0 milestone Dec 29, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.