Checkbox improvements

[ivarne]

When working on Altinn/app-frontend-react#789, I first needed to do some clanup. My first batch of changes was #7718 to remove the shortcut where formData is a string if the component is only configured with a single simpleBinding.

This is my next batch of changes where I try to clan up CheckboxContainerComponent, but unfortuenatly it had to build on the previous pull request that is not yet accepted. I'm not sure if you'd rather want a huge messy PR or multiple smaller, but now you have both.

Description

This PR is intended to fix the following changes in b575af0

• useMemo instead of calculated state
• Fix reference stability issues
• Remove onBlur event (Does not exist on checkbox)
• Remove custom focus functionality (I can't figure out what it does, and how that would be correct)

@jeevananthank I spent most of yesterday trying to figure out the issue in #3782 that the special focus handeling was introduced to try to fix. In this PR I removed the functionality, and I could not reproduce the issue afterwards. Would you be able to have a look?

Fixes

• part of Analsysis: Should we allow (in a standardised way) checkbox support for individual data model fields for each alternative app-frontend-react#789

Verification

• Your code builds clean without any errors or warnings
• Manual testing done (required)
• Relevant automated test added (if you find this hard, leave it and we'll help out)
• All tests run green

Documentation

• User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)
• Changelog is updated with a separate linked PR
  • altinn-app-frontend- (if applicable)
  • nuget-packages - (if applicable)

[jeevananthank]

@ivarne #3782 is not reproducible in the updated code. but our automated tests are failing. Required fields are not validated on navigating between the form components and the error message: 'Feltet er påkrevd' is not presented to the user on navigation.

[ivarne]

@jeevananthank Can you share more details? Its really hard to diagnose a failing test that I don't have access to (as far as I can see), and in code I wouldn't think my changes would affect.

[jeevananthank]

@jeevananthank Can you share more details? Its really hard to diagnose a failing test that I don't have access to (as far as I can see), and in code I wouldn't think my changes would affect.

have a input field with required: true in the app -> point app to localhost version of app frontend -> Start the app instance -> tab through the input field -> error message that says the field is required is not presented.

[ivarne]

Any chance you could have another look at this PR now that #7718 is merged.

[haakemon]

I think this should be removed. If we later are introducing other dependencies that are needed, this eslint-disable might cause a bug since we might forget to add it to the list.

It looks to me it doesnt matter if this is run several times either, because you will only call the handleDataChange if you have not selected anything, so removing it should be fine I think.

[ivarne]

So you think I should put handleDataChange in the dependency list, and just let the effect run on every render (it is not referentially stable)?

It might not be a catastrophe, but definetly seems like the wrong code to me. I could use a ref to make it still be correct

//Library Function
const usePrevious = (value:T)=>{
    const ref = React.useRef<T>(null);
    const previous = ref.current;
    ref.current = value;
    return previous;
};

---
// In render
const previousOptions = usePrevious(options);
React.useEffect(() => {
    const preselectedOption = options?.[props.preselectedOptionIndex]?.value;
    if (
      !selected &&
      options === previousOptions &&
      preselectedOption
    ) {
      props.handleDataChange(preselectedOption);
    }
}, [options, selected, props.handleDataChange]

[haakemon]

Yes, I think handleDataChange should be set as a dependency. Ignoring it may lead to stale closures, which is very bad. Listing it may cause "unnescessary renders" because of the issue that its not referentially stable. If the unnescessary rerenders are causing performance issues, then that should be looked into, and the fix would probably be to wrap the handleDataChange in a useCallback. But if its not causing performance issues, I wouldnt worry about it.

[ivarne]

Stale closures is a non-issue with useEffect, because we create a fresh closure on every render, regardless of the deps array. The question is how we decide when the effect should be executed. This effect needs to run only when the list of options changes, so that we can select the default option if nothing was previously selected. If options did not change, there is no need to update the state. The easiest way to ensure that requirement is to ensure that options is referentially stable and put only options in the deps array.

[haakemon]

Stale closures is a non-issue with useEffect, because we create a fresh closure on every render, regardless of the deps array

The execution of the hook will be skipped, unless any items in the dependency array changed (or if you have an empty array, it will only be run once. If you dont provide an array, it will be run on every render). Stating that stale closures is a non-issue for useEffect is not correct.

This is why the exhaustive-deps rule exists -it is important to list all the dependencies, even functions, so the useEffect runs when something changes, to avoid stale closures.

This effect should run also when props.handleDataChange is changed as well. Some other code might run and change that function to do something else, f.ex call another API. If this useEffect does not react to that change, it will still call the old props.handleDataChange function. The example is a bit contrived, and its highly unlikely that the handleDataChange will do something else at some point, but it illustrates the point.

Adding the comment to disable the exhaustive deps might be fine right now, because we know that props.handleDataChange will in fact not change. But it creates problems later, when some new code is introduced here, that uses other props that may change between renders. You might not notice the comment is there and the check is disabled so you forget to list the new dependency, and you introduce a stale closure and potentially a bug.

You already have a check to see if an option is selected in the useEffect, so it should not matter if the effect is executed on every render.

[ivarne]

No, the examples you link to sets callbacks in useEffect and obviously those callbacks needs to be updated in case they change, because they will be called at a much later time. The closure passed to useEffect here calls handleDataChange immediatly after render and there is no chance that reference to be stale.

The comment to disable the check now is fine because we know handleDataChange will change.

I'm tired of this discussion. Can you just add a suggestion to how you want the code to be written, so I can report the behavioural errors as bugs instead?

[haakemon]

I'm tired of this discussion. Can you just add a suggestion to how you want the code to be written, so I can report the behavioural errors as bugs instead?

Whats with the attitude? I spend time on reviewing the code you wrote, and called out a problem - I think these types of replies are uncalled for. I just want to make sure we dont introduce new potential issues in the code, and not specifying the correct dependencies to the hooks array is a problem.

Either I am not understanding you correctly, or there is a misconception about how useEffect works. But I think the docs are pretty clear on this; if you do not specify every external dependency in the dependency array, the outcome is unpredictable. It does not matter that it "calls handleDataChange immediately after a render", because the entire useEffect hook might be skipped in the next render, because it only cares if options changes. If handleDataChange is changed and options stay the same, the useEffect will not be run and the current reference to handleDataChange will be stale.

However, as I said earlier, this is not the main problem with the code, since handleDataChange is higly unliketly to be changed. The main problem is future maintainability. New dependencies can be introduced which requires the useEffect to be called again. Without having the safetynet of exhaustive-deps, it is likely that this new dependency will be forgotten to be added. This is not a made up issue, this is something I've seen happen several times. This is also why I'm working on removing all the ignore statements for exhaustive-deps.

[ivarne]

Can you please show me the code you believe to be correct? I have shown my attempt previously in this thread, but it requires an extra useRef (hidden in usePrevious), which I don't think adds any clarity or maintainability. Do you think my attempt is wrong in any way? As you see it includes props.handleDataChange, so that is not the big issue.

I'm tired of you not understanding that this effect is only intended to run once after the options have been loaded, in order to set the default choice (preselectedOptionIndex). When that option is set, the user should be allowed to deselect the option and pick another option (or leave it empty) if they want to.

PS: now that I think about it, the preselectedOptionIndex does not make any sense to use in checkbox anyway. For radio buttons it makes total sense, but with checkboxes it should be allowed to leave every box empty, and with the current implementation the preselected index will be checked again if you load the page with an empty selection.

[haakemon]

It would be good if this handler was renamed to handleDataChanged, as it is reacting to an event, not triggering it. I know we are not entirely consistent on these naming conventions in the codebase, but I am trying to improve that :)

Suggested change

	const onDataChanged = (event: React.ChangeEvent<HTMLInputElement>) => {
	const handleDataChanged = (event: React.ChangeEvent<HTMLInputElement>) => {

[ivarne]

I just used the previous naming. I'll change this when the other issues are resolved.

[ivarne]

Another issue is that we already have a function props.handleDataChange in this component. It would be somewhat confusing to have two functions with only one d in difference.

[haakemon]

I find it a bit difficult to follow all the logic here, and I'm not entirely sure what the details are around this, but from the looks of it, it seems like we are now missing some funcitonality for autofocus? The prop shouldFocus f.ex is not used anywhere.

Perhaps @lorang92 knows this area better and what is needed here?

[ivarne]

Yea, I didn't understand what this was doing either. The previous code mixes indexes in the selected array and the options array, so I'm pretty sure it is only correct by chance in some small special cases.

[lorang92]

@ivarne / @haakemon i'm not familiar with this part of the code either. I can't really make sense of it. If regression tests run green my hunch here is that this could be removed.

[lorang92]

@jeevananthank just verify that this issue has not resurfaced when testing: #3782

[lorang92]

@ivarne ah, I see that you already mentioned that ❤️

[jeevananthank]

@lorang92 #3782 is not reproducible in the updated code