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

fix: fix the menu item to navigate to a form to hide when 'hidden' ticked #2426

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

teboho
Copy link
Contributor

@teboho teboho commented Nov 21, 2024

fix the menu item to navigate to a form to hide when 'hidden' ticked

before:
image

after:
image


  • The fact that this is a one line fix is mind-boggling
  • I thought this works because the push function is slower than the concat function, it turns out it's actually faster and that speed was not the problem
  • Perhaps the fact that the concat function creates a new array (a new object instance, while discarding the old one) each time helps

@teboho
Copy link
Contributor Author

teboho commented Nov 21, 2024

Related issue #2268

@teboho
Copy link
Contributor Author

teboho commented Nov 22, 2024

I just asked ChatGPT why this works and the hunch seems correct..

In React and state management, using itemsToCheck.concat([item]) is often better than itemsToCheck.push(item) because of immutability and predictable state updates. Here's why:


1. React State Should Be Immutable

React relies on detecting changes in state to decide when to re-render components. If you mutate an array (e.g., using push), the reference to the array remains the same, and React may not detect that the state has changed.

  • push Mutates the Original Array:

    itemsToCheck.push(item); // Modifies the original array
    setItemsToCheck(itemsToCheck); // React might not detect the change
  • concat Returns a New Array:

    const newArray = itemsToCheck.concat([item]); // Creates a new array
    setItemsToCheck(newArray); // React reliably detects the change

By using concat, you ensure the state change is properly detected because React sees a new array reference.


2. Functional Updates in React

When updating state, React encourages immutability to avoid unintended side effects. Directly mutating the existing array (e.g., with push) can lead to bugs if other parts of the code or components rely on the old state.

  • concat Example:
    setItemsToCheck((prevItems) => prevItems.concat([item]));

This ensures that the new state is derived from the previous state in an immutable way.


3. Debugging and Predictability

Mutating an array with push can lead to unexpected bugs because the same array reference is shared across your application. If another part of your app modifies the same array, it can cause unpredictable behavior.

  • With concat:
    You always work with a new array, reducing the chance of unintended side effects.

4. Pure Functions in State Management

React's state updates are designed to work best with pure functions (functions without side effects). concat adheres to this principle, while push violates it by mutating the array.


Summary

When dealing with React state:

  • Use concat to create a new array and preserve immutability.
  • Avoid push to prevent mutating the existing state, ensuring React detects changes and updates the UI correctly.

Code Comparison:

Using concat (Recommended):

const addItem = (item) => {
  setItemsToCheck((prevItems) => prevItems.concat([item]));
};

Using push (Not Recommended):

const addItem = (item) => {
  itemsToCheck.push(item); // Mutates the array
  setItemsToCheck(itemsToCheck); // React may not detect the change
};

Copy link
Contributor

@AlexStepantsov AlexStepantsov left a comment

Choose a reason for hiding this comment

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

Your changes brokes functionality of checking form access mode.

if you want to use concat then you should change this as itemsToCheck = itemsToCheck.concat([item]);
But with such changes, it will also not work, as it did not work before.

image

Btw, there is also a bug in itemsToCheck.concat(getItemsWithFormNavigation(item.childItems));
shold be
itemsToCheck = itemsToCheck.concat(getItemsWithFormNavigation(item.childItems));

The bug is in something else

@teboho teboho marked this pull request as draft November 24, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants