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.

Sign up for GitHub

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

Jump to bottom

[DataGrid] All selectors accept only apiRef as first argument #16198

Merged
merged 31 commits into from
Feb 13, 2025
Merged

[DataGrid] All selectors accept only apiRef as first argument #16198

merged 31 commits into from
Feb 13, 2025
+781 −761

Conversation

arminmeh
Copy link
Contributor

@arminmeh arminmeh commented Jan 15, 2025 •
edited
Loading

Closes #11440

Changelog

Breaking changes

  • The selectors signature has been updated. They are only accepting apiRef as a first argument.

    -mySelector(state, arguments, instanceId)
+mySelector(apiRef, arguments)

@mui-bot
Copy link

mui-bot commented Jan 15, 2025 •
edited
Loading

Deploy preview: https://deploy-preview-16198--material-ui-x.netlify.app/

Updated pages:

Generated by 🚫 dangerJS against eaff51d

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 20, 2025
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 22, 2025
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 24, 2025
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Feb 5, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 5, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 6, 2025
@arminmeh arminmeh mentioned this pull request Feb 6, 2025
Merged
1 task
@arminmeh arminmeh added breaking change component: data grid This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature labels Feb 7, 2025
@arminmeh arminmeh marked this pull request as ready for review February 10, 2025 09:38
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 10, 2025
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 10, 2025
arminmeh
arminmeh commented Feb 11, 2025
View reviewed changes
docs/data/data-grid/state/state.md
);
```

To make the developer experience better, selectors are typed to allow `apiRef` to reference a `null` value, but they throw an error if called before the state is initialized. Use selectors after the initialization in a `useEffect()` hook or in an event handler.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this to allow using selectors with the apiRef retrieved from useGridApiRef(). It opens the room for an error, but I think it is still better than casting apiRef any time you need to pass it to the selector.

arminmeh and others added 26 commits February 13, 2025 08:59
@arminmeh
Adjust selectors doc generation script
a75fc5c
@arminmeh
Build apiRef from the state param
e4f610a
@MBilalShafi @arminmeh
Test custom memoize
c421cdc
@arminmeh
Use internal type for ref
3b6d62f
@arminmeh
Update selectors that were added on the main branch
a9a24ec
@arminmeh
Fix lint
e33ad75
@MBilalShafi @arminmeh
Add reference and a comment in weakMapMemoize for added context
3ec4401
@MBilalShafi @arminmeh
Get rid of additional details and update naming
1e40983
@arminmeh
Use state as main generic for selectors
5699b29
@arminmeh
Adjust createSelector types
85476e6
@arminmeh
Fix build
af71898
@arminmeh
lint
fc256c1
@arminmeh
Fix selector docs generation
57f47c4
@arminmeh
Update docs and the migration guide
fe421f6
@arminmeh
Update selector examples doc generation
69307b9
@arminmeh
Update migration notes
fd03bec
@arminmeh
Update selectors
8f2ea14
@arminmeh
Revert test changes
80d79bf
@arminmeh
use util to create the selector
45a827c
@arminmeh
rebase fix
5499219
@arminmeh
Wrap initial selectors with a root selector that allows passing apiRe…
18ac918 
…f retrieved from the hook
@arminmeh
small tweaks
66bfe89
@arminmeh
Export fix
ab1f962
@arminmeh
Fix rebase
6a80539
@arminmeh
Rename selector
a5616a0
@arminmeh
do not recreate apiRef when not needed
eaff51d
romgrk
romgrk reviewed Feb 13, 2025
View reviewed changes
packages/x-data-grid/src/utils/createSelector.ts
@@ -12,23 +11,19 @@ const reselectCreateSelector = createSelectorCreator({
maxSize: 1,
equalityCheck: Object.is,
},
argsMemoize: customWeakMapMemoize,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we need to add a custom implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MBilalShafi actually fixed this
reselect version didn't catch the updates inside apiRef of the dependency selectors, so the return values would be inaccurate

Copy link
Member

Choose a reason for hiding this comment

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

The original function weakMapMemoize doesn't memoize the apiRef argument properly due to the static nature of refs, causing in reselect wrapped selectors to not work properly, the custom implementation causes to use apiRef.current.state for comparison rather than apiRef itself.

@arminmeh arminmeh merged commit f854ac1 into mui:master Feb 13, 2025
22 checks passed
@arminmeh arminmeh deleted the align-selectors branch February 13, 2025 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romgrk romgrk romgrk approved these changes

@MBilalShafi MBilalShafi MBilalShafi approved these changes

@cherniavskii cherniavskii Awaiting requested review from cherniavskii

Assignees
No one assigned
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[datagrid] Refactor: make all selectors accept the same argument
5 participants
@arminmeh @mui-bot @romgrk @MBilalShafi @cherniavskii