[DataGrid] Send warning when the rowCount is provided with server

[alexfauquette]

Fix #3602

The first commit is made to send a warning if the paginationMode="server" and the rowCount is undefined. This should help developers to fix the bug.

About the default behavior of Appolo (having data undefined during loading), the only solution I found is to keep the previous rowCount when it moves from a number to undefined. I coded it for discussion, but I think it's better to ask for developers to provide clean props than desynchronize the state and the props of the DataGrid

[mui-bot]

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	284.8	509.8	387.9	397	91.704
Sort 100k rows	ms	536.6	943	805.4	780.14	134.211
Select 100k rows	ms	188.5	314.9	220.9	231	46.244
Deselect 100k rows	ms	124.1	283.9	168.3	183.3	53.457

Generated by 🚫 dangerJS against eadb890

[flaviendelangle]

In wrapWithDeprecationWarning I am doing the env check inside the method, which I think makes the product-code more readable.

And I propose we move the wrapWithDeprecationWarning in this file as well and rename it wrapWithWarning as there goal is very similar.

[alexfauquette]

You can do so because you don't have a if condition to verify before sending the warning.

Since it is a deprecated function, as soon as the function is called you log the message saying "hey it's deprecated"

Here it s not the case, I have a custome if between the env check and the warning. So the wrapper will need to get a function computing the condition in which warning must be logged a bit like that, but not sure it makes the code clearer

export const buildConsistencyWarning = (message: string | string[], warningCondition: () => boolean) => {
  if (process.env.NODE_ENV === 'production') {
    return () => null
  }
  let alreadyWarned = false;

  const cleanMessage = Array.isArray(message) ? message.join('\n') : message;

  return (...params) => {
    if (!alreadyWarned && warningCondition(...params)) {
      alreadyWarned = true;
      console.warn(cleanMessage);
    }
  };
};

[flaviendelangle]

It has the benefits of hiding the env topic in a single file and be sure to never forget it.
But as you like, I agree that the gain is less clear that for the deprecation for the reason you explained.

[flaviendelangle]

I don't know what the best behavior is here.
Could we consider that when loading={true} we don't reset some props when they switch to undefined ?

[alexfauquette]

I'm not in favor of using loading prop to adapt the internal behavior.

For now, loading is only about displaying the loading overlay. Not adding custom logic. It allows developers to use it with no side effects.

[github-actions]

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

[github-actions]

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

[flaviendelangle]

I think it's better to ask for developers to provide clean props than desynchronize the state and the props of the DataGrid

Agreed

[flaviendelangle]

I suggest to:

• Remove this logic
• Add a doc note on the pagination server saying that if the rowCount switch to undefined while loading, users must to the trick you implemented here
• Implement the same trick but in our server pagination demo (and change the fake server behavior to return "undefined" while loading
• Change the warning to link to the doc note

[alexfauquette]

🤯 That's a mind-blowing suggestion:
creating a horrible demo to let users know how to do 😄

[flaviendelangle]

"Horrible", it's the Apollo Client behavior 😆
But alternatively, we can add a new "Apollo Client" section. It would make sense to start giving examples with various famous data management libraries.

[alexfauquette]

I moved the message to the level of an 'error' because it is more important now that we do not correct it by magic.

[github-actions]

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

[github-actions]

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