Skip to content

Commit

Permalink
TextInput: Minimize Allocation of Imperative Functions
Browse files Browse the repository at this point in the history
Summary:
While refactoring `TextInput`, I noticed that we are allocating each of the exported imperative instance methods on every invocation of the component. This is wasteful because if the `ref` does not change, this just creates more garbage.

Instead, we only need to allocate them when we are actually going to assign them onto a `ref`.

Changelog:
[Internal]

Reviewed By: sammy-SC

Differential Revision: D41191440

fbshipit-source-id: 0120d67a9e29a3e0c3f42c3fa436381d6bec7619
  • Loading branch information
yungsters authored and facebook-github-bot committed Nov 12, 2022
1 parent 9329121 commit 7bcc6f9
Showing 1 changed file with 60 additions and 65 deletions.
125 changes: 60 additions & 65 deletions Libraries/Components/TextInput/TextInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -1184,75 +1184,70 @@ function InternalTextInput(props: Props): React.Node {
}
}, [inputRef]);

function clear(): void {
if (inputRef.current != null) {
viewCommands.setTextAndSelection(
inputRef.current,
mostRecentEventCount,
'',
0,
0,
);
}
}

function setSelection(start: number, end: number): void {
if (inputRef.current != null) {
viewCommands.setTextAndSelection(
inputRef.current,
mostRecentEventCount,
null,
start,
end,
);
const setLocalRef = (ref: TextInputInstance | null) => {
inputRef.current = ref;

/*
Hi reader from the future. I'm sorry for this.
This is a hack. Ideally we would forwardRef to the underlying
host component. However, since TextInput has it's own methods that can be
called as well, if we used the standard forwardRef then these
methods wouldn't be accessible and thus be a breaking change.
We have a couple of options of how to handle this:
- Return a new ref with everything we methods from both. This is problematic
because we need React to also know it is a host component which requires
internals of the class implementation of the ref.
- Break the API and have some other way to call one set of the methods or
the other. This is our long term approach as we want to eventually
get the methods on host components off the ref. So instead of calling
ref.measure() you might call ReactNative.measure(ref). This would hopefully
let the ref for TextInput then have the methods like `.clear`. Or we do it
the other way and make it TextInput.clear(textInputRef) which would be fine
too. Either way though is a breaking change that is longer term.
- Mutate this ref. :( Gross, but accomplishes what we need in the meantime
before we can get to the long term breaking change.
*/
if (ref != null) {
// $FlowFixMe[incompatible-use] - See the explanation above.
Object.assign(ref, {
clear(): void {
if (inputRef.current != null) {
viewCommands.setTextAndSelection(
inputRef.current,
mostRecentEventCount,
'',
0,
0,
);
}
},
// TODO: Fix this returning true on null === null, when no input is focused
isFocused(): boolean {
return TextInputState.currentlyFocusedInput() === inputRef.current;
},
getNativeRef(): ?React.ElementRef<HostComponent<mixed>> {
return inputRef.current;
},
setSelection(start: number, end: number): void {
if (inputRef.current != null) {
viewCommands.setTextAndSelection(
inputRef.current,
mostRecentEventCount,
null,
start,
end,
);
}
},
});
}
}

// TODO: Fix this returning true on null === null, when no input is focused
function isFocused(): boolean {
return TextInputState.currentlyFocusedInput() === inputRef.current;
}

function getNativeRef(): ?React.ElementRef<HostComponent<mixed>> {
return inputRef.current;
}
};

const _setNativeRef = setAndForwardRef({
getForwardedRef: () => props.forwardedRef,
setLocalRef: ref => {
inputRef.current = ref;

/*
Hi reader from the future. I'm sorry for this.
This is a hack. Ideally we would forwardRef to the underlying
host component. However, since TextInput has it's own methods that can be
called as well, if we used the standard forwardRef then these
methods wouldn't be accessible and thus be a breaking change.
We have a couple of options of how to handle this:
- Return a new ref with everything we methods from both. This is problematic
because we need React to also know it is a host component which requires
internals of the class implementation of the ref.
- Break the API and have some other way to call one set of the methods or
the other. This is our long term approach as we want to eventually
get the methods on host components off the ref. So instead of calling
ref.measure() you might call ReactNative.measure(ref). This would hopefully
let the ref for TextInput then have the methods like `.clear`. Or we do it
the other way and make it TextInput.clear(textInputRef) which would be fine
too. Either way though is a breaking change that is longer term.
- Mutate this ref. :( Gross, but accomplishes what we need in the meantime
before we can get to the long term breaking change.
*/
if (ref != null) {
Object.assign(ref, {
clear,
isFocused,
getNativeRef,
setSelection,
});
}
},
setLocalRef,
});

const _onChange = (event: ChangeEvent) => {
Expand Down

0 comments on commit 7bcc6f9

Please sign in to comment.