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

add initial previous value param to usePrevious hook #316

Closed

Conversation

wherehows
Copy link

@wherehows wherehows commented Jul 4, 2024

Summary

Add optional initial value parameter to usePrevious hook and fix typescript

Reason

1. Performance Issue

The current implementation uses two useState hooks and updates state during render, which can cause unnecessary re-renders and violate React's rendering rules:

const [current, setCurrent] = React.useState(value);
const [previous, setPrevious] = React.useState(null);
if (value !== current) { // State updates during render
  setPrevious(current);
  setCurrent(value);
}

2. Initial Value

The hook always returns null as the initial previous value, which causes issues in scenarios where a different initial value is needed:

function MyTable({data, loading}) {
  const previousData = usePrevious(data); // Initially returns null
  // This throws an exception when loading is true
  const table = useReactTable({data: loading ? previousData : data}); 
}

3. Wrong Type

Current type definition doesn't accurately represent the hook's behavior:

export function usePrevious<T>(newValue: T): T; // Current
export function usePrevious<T>(newValue: T): T | null; // Should be

@@ -1023,7 +1023,7 @@ export function usePrevious(value) {
setCurrent(value);
}

return previous;
return previous || initialPreviousValue;

Choose a reason for hiding this comment

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

This works as default value, not the initial. Imagine calling usePrevious<number>(value, 1) with the value being: 1, 2, 0, 3. And we'll see 1, 1, 2, 1, right? It would be nice to have some tests confirming it works for some inputs...

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