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

Common select fixes #1449

Merged
merged 3 commits into from
Mar 21, 2024
Merged

Common select fixes #1449

merged 3 commits into from
Mar 21, 2024

Conversation

Hyperkid123
Copy link
Member

Fixes #1448

Description

  • default options prop resetting select state if not cached
  • compareValues included in select promises payload
  • select updateOptions dispatch triggering on with same options

@Hyperkid123 Hyperkid123 added bug Something isn't working common labels Mar 21, 2024
@Hyperkid123 Hyperkid123 requested a review from rvsia March 21, 2024 08:44
Copy link

vercel bot commented Mar 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-forms ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 21, 2024 11:08am

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 94.49%. Comparing base (271e466) to head (23a38f5).

Files Patch % Lines
packages/common/src/use-select/reducer.js 50.00% 2 Missing ⚠️
packages/common/src/use-select/use-select.js 88.88% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1449   +/-   ##
=======================================
  Coverage   94.48%   94.49%           
=======================================
  Files         210      210           
  Lines        3971     3975    +4     
  Branches     1628     1629    +1     
=======================================
+ Hits         3752     3756    +4     
  Misses        219      219           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@haydenlinder
Copy link

🙏

@@ -69,9 +69,16 @@ const useSelect = ({
simpleValue,
compareValues,
}) => {
const [state, originalDispatch] = useReducer(reducer, { optionsTransformer, propsOptions }, init);
const [propsOptions, setPropsCache] = useState(initialOptions);
const [state, originalDispatch] = useReducer(reducer, { optionsTransformer, propsOptions: initialOptions }, init);
const dispatch = (action) => originalDispatch({ ...action, optionsTransformer });
Copy link
Contributor

Choose a reason for hiding this comment

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

We can pass the compareValues here so no need to pass it to all following dispatches

Comment on lines 124 to 125
if (!isEqual(state.options, propsOptions)) {
if (state.isInitialLoaded) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These two ifs can be merged

const dispatch = (action) => originalDispatch({ ...action, optionsTransformer });

useEffect(() => {
if (!isEqual(initialOptions, propsOptions)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it lead to performance issues? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

For massive arrays of options it could, but as it stands infinite render loops are way worse.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, they are 😄

@rvsia rvsia merged commit 90f2774 into master Mar 21, 2024
5 of 6 checks passed
@rvsia rvsia added the released label Mar 21, 2024
@Hyperkid123 Hyperkid123 deleted the mui-searchable-async-select branch March 21, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working common released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding isSearchable to MUI Select with loadOptions breaks loading
3 participants