[core] Generate propTypes #2395
Conversation
| } | ||
| return null; | ||
| }), | ||
| checkboxSelectionVisibleOnly: chainPropTypes(PropTypes.bool, (props: any) => { |
There was a problem hiding this comment.
We are losing the custom prop-types. If necessary, we can use this comment https://github.com/mui-org/material-ui/blob/a4e2ed82c76fffb5875ba96ab8c0efd5bcb424ef/packages/material-ui/src/Popper/Popper.js#L314
x the others
There was a problem hiding this comment.
The comment doesn't work in this situation. I'm already using it on other props. The prop-type for this prop was removed because checkboxSelectionVisibleOnly is omitted from the grid's props. The generator doesn't allow to have more prop-types than the props specified in the TS definition.
There was a problem hiding this comment.
What is the conclusion here ?
Do we have a way to improve the generator to keep those prop types ?
There was a problem hiding this comment.
There's no way to include more props than those allowed in the interface of the component. In theory, if the prop is not in the interface it shouldn't be used in the component. We could add a warning in useDataGridProps if the user tries to use a prop from the Pro version.
There was a problem hiding this comment.
Would you do this switch to useDataGridProps / useDataGridProProps in this PR ?
It would be sad to have a release without those warnings.
There was a problem hiding this comment.
Would you do this switch to useDataGridProps / useDataGridProProps in this PR ?
In another PR.
package.json
Outdated
| @@ -61,6 +62,7 @@ | |||
| "@material-ui/core": "^4.9.12", | |||
| "@material-ui/icons": "^4.11.2", | |||
| "@material-ui/lab": "^4.0.0-alpha.54", | |||
| "@material-ui/utils": "^5.0.0-beta.4", | |||
There was a problem hiding this comment.
I don't know why but without adding it here, the build fails. The dependency is already added to the DataGrid's package.json.
There was a problem hiding this comment.
- Saw a new warning opening http://0.0.0.0:3001/components/data-grid/columns/:
- I expect the new prop-types to significantly increase the bundle size. But we didn't bring the bundle size checker from the main repo yet so we are blind. Also, we are no using the babel remove prop-type helpers to work around the limitation. What should do about it? Would it make more sense to only generate the prop-types for
DataGridandDataGridProand hold on for the other components?
| // Fix const foo = /{{(.+?)}}/gs; crashing. | ||
| /prettier/, |
There was a problem hiding this comment.
It seems to work without, is it still needed?
| // Fix const foo = /{{(.+?)}}/gs; crashing. | |
| /prettier/, |
There was a problem hiding this comment.
Here it crashes if I remove.
| before(() => { | ||
| PropTypes.resetWarningCache(); | ||
| }); |
There was a problem hiding this comment.
Seeing this diff made me suspicious, so I gave it a try to confirm it: When running the tests in watch mode, the first run is green, but the second one is red. What motived the change of approach?
There was a problem hiding this comment.
To test the warnings we were using PropTypes.checkPropTypes and feeding it with the prop-types. However, to get the prop-types they had to be attached to the exported component, which is a memo. Memoized components can't have prop-types so the generator attaches them to the DataGridRaw.
Since there's no more way to access the prop-types directly, I had to render the component to be able to test. The reason for seeing the second run red in watch mode: facebook/react#18251 (comment)
@oliviertassinari It increased 5%. I'll open another PR to remove during the build the prop-types from internal components.
|
One step towards #1826
I'm already working to generate the documentation for the components based on the propTypes.