[DongEun02]

Overview

Refs #553

Previously, the transformValue callback in useInputState only received the newly entered value, making it difficult to reference the state immediately before an update during transformation.

To address this, previousValue is now provided as the second argument to transformValue. Using only nextValue remains the primary usage, while previousValue can be used when needed.

Internally, a functional state update is used to provide the state immediately before the update.
I also considered using usePrevious, but it returns the value from the previous render rather than the state immediately before an update, so it did not fit this use case.

Checklist

• Did you write the test code?
• Have you run yarn run fix to format and lint the code and docs?
• Have you run yarn run test:coverage to make sure there is no uncovered line?
• Did you write the JSDoc?

[changeset-bot]

🦋 Changeset detected

Latest commit: 560e3ee

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
react-simplikit	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (cf0dd83) to head (560e3ee).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #574   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           69        69           
  Lines         2281      2284    +3     
  Branches       737       736    -1     
=========================================
+ Hits          2281      2284    +3     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Antoliny0919]

Nice @DongEun02 🙌

It looks good to me, I left very small suggestion :)

[Antoliny0919]

The default function’s parameter was renamed in the documentation, but the implementation still uses the old name.

Before

[transformValue=(v: string) => v]

After

[transformValue=(nextValue: string) => nextValue]

Would it make sense to update the implementation for consistency? I think the echo function should be updated as well.

function echo(nextValue: string) {
  return nextValue;
}

[DongEun02]

Thanks for pointing this out! 👍

I agree that the implementation should be consistent with the documentation.
I update the parameter name in the default function and the echo function as well.

[hyesungoh]

thanks for this 👍

API looks fine, just few request on the implementation

could we apss the current value from the closure instead of using an updater?

// ❌ transformValue runs inside the state updater
setValue(previousValue => transformValue(nextValue, previousValue));

// ✅ transformValue runs in the event handler, like before
setValue(transformValue(nextValue, value));
// deps: [transformValue, value]

React expects updaters to be pure, so moving transformValue into one changes when it runs. Pure transforms like the length limit in #553 behave the same, but transforms that also do something (show a toast, call a parent's setter, log) don't:

1. In StrictMode, transformValue runs twice per change.
2. If another update is already queued in the same event, it runs during render, so calling a parent's setter there warns Cannot update a component while rendering a different component.

The closure only falls short when onChange is called twice in one handler, which doesn't really happen with input events.

Could you add tests like these? The first two fail with the updater, and the last one fails if value is missing from the deps.

import { ChangeEvent, createElement, StrictMode, useState } from 'react';

it('should call transformValue once per change in StrictMode', () => {
  const transformValue = vi.fn((nextValue: string) => nextValue);
  const { getByRole } = render(createElement(StrictMode, null, createElement(createTestInput('', transformValue))));

  fireEvent.change(getByRole('textbox'), { target: { value: 'a' } });

  expect(transformValue).toHaveBeenCalledTimes(1);
});

it('should let transformValue update another component', () => {
  const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {});

  function Input({ onTooLong }: { onTooLong: (tooLong: boolean) => void }) {
    const [, setTouched] = useState(false);
    const [value, onChange] = useInputState('', (nextValue, previousValue) => {
      onTooLong(nextValue.length > 3);
      return nextValue.length > 3 ? previousValue : nextValue;
    });

    return createElement('input', {
      value,
      onChange: (event: ChangeEvent<HTMLInputElement>) => {
        setTouched(true);
        onChange(event);
      },
    });
  }

  function Form() {
    const [tooLong, setTooLong] = useState(false);
    return createElement('div', null, createElement(Input, { onTooLong: setTooLong }), tooLong ? 'too long' : '');
  }

  const { getByRole, getByText } = render(createElement(Form));
  fireEvent.change(getByRole('textbox'), { target: { value: 'abcd' } });

  expect(getByText('too long')).toBeInTheDocument();
  expect(consoleError).not.toHaveBeenCalled();
  consoleError.mockRestore();
});

it('should pass the latest state as previousValue', () => {
  const { getByRole } = render(
    createElement(createTestInput('', (nextValue, previousValue) => (nextValue.length > 3 ? previousValue : nextValue)))
  );
  const input = getByRole('textbox');

  fireEvent.change(input, { target: { value: 'abc' } });
  fireEvent.change(input, { target: { value: 'abcd' } });

  expect(input).toHaveValue('abc');
});

Also, could you bump the changeset to minor?

We've been releasing new options like this as minor

[DongEun02]

Thanks for the detailed review!

I updated onChange to use the current value from the closure and added value to the callback dependencies.
I also added the three requested tests and bumped the changeset to minor.

[hyesungoh]

This works well I think. but one thing that im not sure about it

'onChange gets a new reference every time the value change'

(since value is now in deps)

we could keep it stable with 'usePreservedCallback'
but it syncs the latest callback in 'useEffect' so Im a little worried it could have some timing edge case 🤔

what do you think about it?

[DongEun02]

As you pointed out, the current implementation creates a new onChange reference whenever value changes.

I tried using usePreservedCallback. I didn’t observe an issue during normal typing, but I could reproduce the timing edge case by calling the preserved handler from a useLayoutEffect, before usePreservedCallback updated its callback ref in useEffect.

Here’s the relevant part of the reproduction.

const [value, setValue] = useState('abcd');
const triggered = useRef(false);

const onChange = usePreservedCallback((nextValue: string) => {
  setValue(transformValue(nextValue, value));
});

useLayoutEffect(() => {
  if (value === 'abcde' && !triggered.current) {
    triggered.current = true;
    onChange('abcdef');
  }
}, [value, onChange]);

After typing e, value was abcde when the layout Effect ran, but transformValue received abcd as previousValue.

(This reproduction deliberately calls the handler during a layout Effect. I didn’t observe the issue during normal typing, so I can’t say how likely it is in typical usage.)

Given that result, I think it’s better to accept a new onChange reference when value changes and keep the current implementation so it passes the correct previousValue.