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.

Sign up for GitHub

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

remove use of findDOMNode in number-input #1915

Merged
merged 3 commits into from
Nov 27, 2024
Merged

remove use of findDOMNode in number-input #1915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8eeda49
remove use of findDOMNode in number-input
handeyeco Nov 26, 2024
f64d733
changeset
handeyeco Nov 26, 2024
61e40a1
respond to Jeremys feedback
handeyeco Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/chilly-carrots-wink.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

Remove use of findDOMNode in number-input component
57 changes: 25 additions & 32 deletions packages/perseus/src/components/number-input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
/* eslint-disable @khanacademy/ts-no-error-suppressions */
import {number as knumber} from "@khanacademy/kmath";
import {Errors, PerseusError} from "@khanacademy/perseus-core";
import classNames from "classnames";
import $ from "jquery";
import PropTypes from "prop-types";
import * as React from "react";
import ReactDOM from "react-dom";
import _ from "underscore";

import Util from "../util";
Expand Down Expand Up @@ -40,6 +38,7 @@ const getNumericFormat = KhanMath.getNumericFormat;
class NumberInput extends React.Component<any, any> {
static contextType = PerseusI18nContext;
declare context: React.ContextType<typeof PerseusI18nContext>;
inputRef = React.createRef<HTMLInputElement>();

static propTypes = {
value: PropTypes.number,
Expand Down Expand Up @@ -71,20 +70,27 @@ class NumberInput extends React.Component<any, any> {
}
}

_getInput: () => HTMLInputElement = () => {
if (!this.inputRef.current) {
throw new PerseusError(
"Input ref accessed before set",
Errors.Internal,
);
Comment on lines +75 to +78
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this migration to use a real Ref. One concern is that this function could get called by something after dismounting and we start throwing errors. Perhaps that's fine as an unmounted component throwing errors shouldn't tear down the React tree, right?

Copy link
Contributor Author

@handeyeco handeyeco Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I was a little worried about doing this, but I think it's roughly the same from the learner's perspective if we throw an error intentionally vs getting an error from trying to access a property on an unset value.

For example, the before:

        // @ts-expect-error - TS2531 - Object is possibly 'null'
        return ReactDOM.findDOMNode(this.refs.input).value.toString();

So here we'd get an error when we try to access .value (because I believe findDOMNode would return null). Now we'd still throw an error, but we'd be doing it intentionally and can provide an error message.

Copy link
Contributor

@nishasy nishasy Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think I've seen this approach in Perseus? Can we not just do a this.inputRef.current?.focus(); and leave it at that rather than using a _getInput() function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would require changing the logic and the return types of all the methods that weren't properly checking the ref before. I was worried that would spiral out of control if I then had to handle undefined where number-input was consumed. We do it regularly in Webapp:

        const itemRenderer = itemRendererRef.current;
        if (itemRenderer == null) {
            // NOTE(kevinb): this should never happen since the itemRenderer
            // and the exercise chrome should be unmounted at the same time.
            throw new KAError(
                "cannot check answer on null item renderer",
                Errors.Internal,
            );
        }
        const score = itemRenderer.scoreInput();

}

return this.inputRef.current;
};

/* Return the current "value" of this input
* If empty, it returns the placeholder (if it is a number) or null
*/
getValue: () => any = () => {
return this.parseInputValue(
// @ts-expect-error - TS2531 - Object is possibly 'null'. | TS2339 - Property 'value' does not exist on type 'Element | Text'.
ReactDOM.findDOMNode(this.refs.input).value, // eslint-disable-line react/no-string-refs
);
return this.parseInputValue(this._getInput().value);
};

/* Return the current string value of this input */
getStringValue: () => string = () => {
// @ts-expect-error - TS2531 - Object is possibly 'null'. | TS2339 - Property 'value' does not exist on type 'Element | Text'.
return ReactDOM.findDOMNode(this.refs.input).value.toString(); // eslint-disable-line react/no-string-refs
return this._getInput().toString();
};

parseInputValue: (arg1: any) => any = (value) => {
Expand All @@ -98,36 +104,28 @@ class NumberInput extends React.Component<any, any> {

/* Set text input focus to this input */
focus: () => void = () => {
// @ts-expect-error - TS2531 - Object is possibly 'null'. | TS2339 - Property 'focus' does not exist on type 'Element | Text'.
ReactDOM.findDOMNode(this.refs.input).focus(); // eslint-disable-line react/no-string-refs
this._getInput().focus();
this._handleFocus();
};

blur: () => void = () => {
// @ts-expect-error - TS2531 - Object is possibly 'null'. | TS2339 - Property 'blur' does not exist on type 'Element | Text'.
ReactDOM.findDOMNode(this.refs.input).blur(); // eslint-disable-line react/no-string-refs
this._getInput().blur();
this._handleBlur();
};

setSelectionRange: (arg1: number, arg2: number) => any = (
setSelectionRange: (arg1: number, arg2: number) => void = (
selectionStart,
selectionEnd,
) => {
// @ts-expect-error - TS2531 - Object is possibly 'null'. | TS2339 - Property 'setSelectionRange' does not exist on type 'Element | Text'.
ReactDOM.findDOMNode(this).setSelectionRange(
selectionStart,
selectionEnd,
);
this._getInput().setSelectionRange(selectionStart, selectionEnd);
};

getSelectionStart: () => number = () => {
// @ts-expect-error - TS2531 - Object is possibly 'null'. | TS2339 - Property 'selectionStart' does not exist on type 'Element | Text'.
return ReactDOM.findDOMNode(this).selectionStart;
getSelectionStart: () => number | null = () => {
return this._getInput().selectionStart;
};

getSelectionEnd: () => number = () => {
// @ts-expect-error - TS2531 - Object is possibly 'null'. | TS2339 - Property 'selectionEnd' does not exist on type 'Element | Text'.
return ReactDOM.findDOMNode(this).selectionEnd;
getSelectionEnd: () => number | null = () => {
return this._getInput().selectionEnd;
};

_checkValidity: (arg1: any) => boolean = (value) => {
Expand Down Expand Up @@ -203,11 +201,7 @@ class NumberInput extends React.Component<any, any> {
};

_setValue: (arg1: number, arg2: MathFormat) => void = (val, format) => {
// eslint-disable-next-line react/no-string-refs
// @ts-expect-error - TS2769 - No overload matches this call. | TS2339 - Property 'val' does not exist on type 'JQueryStatic'.
$(ReactDOM.findDOMNode(this.refs.input)).val(
toNumericString(val, format),
);
this._getInput().value = toNumericString(val, format);
};

render(): React.ReactNode {
Expand Down Expand Up @@ -237,8 +231,7 @@ class NumberInput extends React.Component<any, any> {
{...restProps}
className={classes}
type="text"
// eslint-disable-next-line react/no-string-refs
ref="input"
ref={this.inputRef}
onChange={this._handleChange}
onFocus={this._handleFocus}
onBlur={this._handleBlur}
Expand Down