[DataGrid] Grid depends on side effects #666
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oliviertassinari
added
docs
oliviertassinari
force-pushed
the
data-grid-prevent-optimization
branch
4 times, most recently
from
af4fee6 to
14070ff
Compare
oliviertassinari
force-pushed
the
data-grid-prevent-optimization
branch
from
30ae55e to
09982f6
Compare
DanailH
approved these changes
Dec 1, 2020
dtassone
reviewed
Dec 1, 2020
| applyValue, | ||
| type, | ||
| }) => { | ||
| export function FilterInputValue(props: TypeFilterInputValueProps) { |
There was a problem hiding this comment.
I think for a better typescript support, it would better to keep the type definition otherwise it might just see it as a function instead of a react functional component 🤔
There was a problem hiding this comment.
interface FunctionComponent<P = {}> {
(props: PropsWithChildren<P>, context?: any): ReactElement<any, any> | null;
propTypes?: WeakValidationMap<P>;
contextTypes?: ValidationMap<any>;
defaultProps?: Partial<P>;
displayName?: string;
}There was a problem hiding this comment.
I believe none of the added types of FunctionComponent are relevant:
- injects a
childrenprop, a prop that might not be supported or even be correct: [Tooltip] TypeScript issue with children type and React.FunctionComponent material-ui#23634. - displayName is harmful most of the time (increase bundle size, better alternatives available)
- defaultProps is on its way to deprecation
- contextTypes is legacy
- propTypes isn't relevant with types
I would be in favor of following @eps1lon's point in mui/material-ui#23634 (comment)
dtassone
approved these changes
Dec 1, 2020
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fix #644 (comment)
https://babeljs.io/docs/en/babel-plugin-transform-react-constant-elements is used to optimize the rendering, it moves constant React elements to the upper scope, outside of the React function. So that when the component rerenders, the reference to the React element is identifical, this allows React to skip the rendering of the element, it similar to using React.memo.
However, the data grid relies on a side effect, it depends on its parent to systematically render its children to read the latest value of the grid state and correctly update the DOM.
This is a temporary workaround until we refactor the way the state of the grid is handled. State changes should propagate regardless of React rendering pruning (component memorization or element reference equality).