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

[core] Host normalize-scroll-left #19638

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Feb 9, 2020

I have adapted the source to remove the server-side and setter support. The project has almost no feedback from the community (but is maintained), I think that it's a good signal that can consolidate the logic on Material-UI :).

@alitaheri Thank you for working on such a project!

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Feb 9, 2020
@mui-pr-bot
Copy link

Details of bundle changes.

Comparing: f567b38...6a43efe

bundle Size Change Size Gzip Change Gzip
@material-ui/core[umd] ▼ -182 B (-0.06% ) 317 kB ▼ -48 B (-0.05% ) 92 kB
Tabs ▼ -180 B (-0.21% ) 85.6 kB ▼ -76 B (-0.28% ) 27.1 kB
@material-ui/core ▼ -180 B (-0.05% ) 362 kB ▼ -42 B (-0.04% ) 98.8 kB
@material-ui/lab -- 194 kB -- 57.3 kB
@material-ui/styles -- 51.4 kB -- 15.4 kB
@material-ui/system -- 16.4 kB -- 4.3 kB
Alert -- 83.6 kB -- 26.3 kB
AlertTitle -- 64.4 kB -- 20.3 kB
AppBar -- 64.2 kB -- 20.1 kB
Autocomplete -- 132 kB -- 41.4 kB
Avatar -- 65.4 kB -- 20.6 kB
AvatarGroup -- 62.4 kB -- 19.6 kB
Backdrop -- 68 kB -- 21 kB
Badge -- 65.5 kB -- 20.4 kB
BottomNavigation -- 62.6 kB -- 19.6 kB
BottomNavigationAction -- 75.7 kB -- 23.9 kB
Box -- 72 kB -- 21.8 kB
Breadcrumbs -- 67.9 kB -- 21.3 kB
Button -- 79.9 kB -- 24.5 kB
ButtonBase -- 74.2 kB -- 23.3 kB
ButtonGroup -- 83.4 kB -- 25.5 kB
Card -- 63 kB -- 19.7 kB
CardActionArea -- 75.3 kB -- 23.7 kB
CardActions -- 62.2 kB -- 19.5 kB
CardContent -- 62.1 kB -- 19.5 kB
CardHeader -- 65.2 kB -- 20.5 kB
CardMedia -- 62.5 kB -- 19.7 kB
Checkbox -- 83.2 kB -- 26.3 kB
Chip -- 82.8 kB -- 25.4 kB
CircularProgress -- 64.3 kB -- 20.3 kB
ClickAwayListener -- 3.91 kB -- 1.55 kB
Collapse -- 68.2 kB -- 21.1 kB
colorManipulator -- 3.88 kB -- 1.52 kB
Container -- 63.3 kB -- 19.8 kB
CssBaseline -- 57.7 kB -- 18.1 kB
Dialog -- 83.2 kB -- 25.9 kB
DialogActions -- 62.3 kB -- 19.5 kB
DialogContent -- 62.4 kB -- 19.6 kB
DialogContentText -- 64.2 kB -- 20.2 kB
DialogTitle -- 64.4 kB -- 20.2 kB
Divider -- 62.9 kB -- 19.8 kB
docs.landing -- 56.4 kB -- 14.5 kB
docs.main -- 596 kB -- 194 kB
Drawer -- 85 kB -- 25.8 kB
ExpansionPanel -- 72.5 kB -- 22.7 kB
ExpansionPanelActions -- 62.2 kB -- 19.5 kB
ExpansionPanelDetails -- 62.1 kB -- 19.5 kB
ExpansionPanelSummary -- 78.3 kB -- 24.8 kB
Fab -- 77 kB -- 24 kB
Fade -- 23.4 kB -- 7.98 kB
FilledInput -- 73.7 kB -- 22.9 kB
FormControl -- 64.6 kB -- 20.2 kB
FormControlLabel -- 65.7 kB -- 20.6 kB
FormGroup -- 62.2 kB -- 19.5 kB
FormHelperText -- 63.5 kB -- 20 kB
FormLabel -- 63.6 kB -- 19.8 kB
Grid -- 65.3 kB -- 20.5 kB
GridList -- 62.6 kB -- 19.7 kB
GridListTile -- 63.9 kB -- 20 kB
GridListTileBar -- 63.4 kB -- 19.9 kB
Grow -- 24 kB -- 8.19 kB
Hidden -- 66.1 kB -- 20.8 kB
Icon -- 63 kB -- 19.7 kB
IconButton -- 76.3 kB -- 23.8 kB
Input -- 72.7 kB -- 22.7 kB
InputAdornment -- 65.3 kB -- 20.5 kB
InputBase -- 70.8 kB -- 22.2 kB
InputLabel -- 65.5 kB -- 20.5 kB
LinearProgress -- 65.5 kB -- 20.5 kB
Link -- 66.8 kB -- 21.1 kB
List -- 62.5 kB -- 19.5 kB
ListItem -- 77.3 kB -- 24.2 kB
ListItemAvatar -- 62.3 kB -- 19.5 kB
ListItemIcon -- 62.3 kB -- 19.5 kB
ListItemSecondaryAction -- 62.2 kB -- 19.5 kB
ListItemText -- 65.1 kB -- 20.5 kB
ListSubheader -- 62.9 kB -- 19.8 kB
Menu -- 89 kB -- 27.4 kB
MenuItem -- 78.4 kB -- 24.5 kB
MenuList -- 66.2 kB -- 20.7 kB
MobileStepper -- 68 kB -- 21.4 kB
Modal -- 14.5 kB -- 5.04 kB
NativeSelect -- 77 kB -- 24.3 kB
NoSsr -- 2.19 kB -- 1.04 kB
OutlinedInput -- 74.7 kB -- 23.3 kB
Pagination -- 86.2 kB -- 26.3 kB
PaginationItem -- 82.4 kB -- 25.5 kB
Paper -- 62.5 kB -- 19.5 kB
Popover -- 83.3 kB -- 25.8 kB
Popper -- 28.8 kB -- 10.3 kB
Portal -- 2.92 kB -- 1.3 kB
Radio -- 84.2 kB -- 26.5 kB
RadioGroup -- 64.6 kB -- 20.1 kB
Rating -- 70.7 kB -- 22.7 kB
RootRef -- 4.24 kB -- 1.64 kB
Select -- 117 kB -- 34.6 kB
Skeleton -- 63.1 kB -- 20 kB
Slide -- 25.5 kB -- 8.71 kB
Slider -- 76.8 kB -- 24.2 kB
Snackbar -- 75.6 kB -- 23.7 kB
SnackbarContent -- 63.7 kB -- 20.1 kB
SpeedDial -- 86.4 kB -- 27.2 kB
SpeedDialAction -- 119 kB -- 37.5 kB
SpeedDialIcon -- 64.7 kB -- 20.4 kB
Step -- 62.8 kB -- 19.7 kB
StepButton -- 82.6 kB -- 26.1 kB
StepConnector -- 62.9 kB -- 19.8 kB
StepContent -- 69.3 kB -- 21.7 kB
StepIcon -- 64.8 kB -- 20.2 kB
StepLabel -- 68.8 kB -- 21.7 kB
Stepper -- 65.1 kB -- 20.6 kB
styles/createMuiTheme -- 16.5 kB -- 5.81 kB
SvgIcon -- 63.2 kB -- 19.8 kB
SwipeableDrawer -- 92.4 kB -- 28.8 kB
Switch -- 82.3 kB -- 26 kB
Tab -- 76.5 kB -- 24.2 kB
Table -- 62.7 kB -- 19.7 kB
TableBody -- 62.3 kB -- 19.5 kB
TableCell -- 64.2 kB -- 20.2 kB
TableContainer -- 62.1 kB -- 19.5 kB
TableFooter -- 62.3 kB -- 19.5 kB
TableHead -- 62.3 kB -- 19.5 kB
TablePagination -- 144 kB -- 41.9 kB
TableRow -- 62.7 kB -- 19.7 kB
TableSortLabel -- 77.6 kB -- 24.4 kB
TextareaAutosize -- 5.12 kB -- 2.14 kB
TextField -- 125 kB -- 36.6 kB
ToggleButton -- 76.3 kB -- 24.2 kB
ToggleButtonGroup -- 63.4 kB -- 20 kB
Toolbar -- 62.5 kB -- 19.6 kB
Tooltip -- 102 kB -- 32.4 kB
TreeItem -- 74.2 kB -- 23.5 kB
TreeView -- 66.8 kB -- 21.1 kB
Typography -- 63.8 kB -- 20 kB
useAutocomplete -- 14.7 kB -- 5.31 kB
useMediaQuery -- 2.58 kB -- 1.07 kB
Zoom -- 23.5 kB -- 8.09 kB

Generated by 🚫 dangerJS against 6a43efe

@TrySound
Copy link
Contributor

TrySound commented Feb 9, 2020

ESM support was accepted pretty fast

@oliviertassinari
Copy link
Member Author

@TrySound It's definitely maintained, I have no concern on this side.

@eps1lon
Copy link
Member

eps1lon commented Feb 10, 2020

What's the purpose of this PR? every dependency inlined increases app bundle size.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Feb 10, 2020

I think that in an ideal world, Material-UI would have zero dependencies because:

  • It gives us the most freedom, we won't be held back by any of them. We have seen this issue unfold a couple of time, recently, we had strict mode and IE 11 concerns.
  • It tells our users a lot about our mission, what problem we try to solve.
  • It feels safer from our users' perspective.

Over the last few months, we have been going further in this direction, removed a bunch of dependencies (keycode, react-event-listener, react-select, warning, downshift) and we plan to go further down this direction with style adapters.

Now, in practice, having dependencies can also help us (I have tried to list all the deps I'm aware we have):

  • React: we don't have the resources to cover Vue nor Angular yet. We have also made it very explicit that React is our core focus and belief that it will hold and be dominant in the future.
  • jss: we are moving away from any hard dependencies on the styles, with the goal to support multiples engies.
  • prop-types: industry standard.
  • react-is: industry standard.
  • @babel/runtime: industry standard.
  • hoist-non-react-statics: industry standard.
  • clsx: hope it will become an industry standard.
  • normalize-scroll-left: there are very few lines of code, the problem is solved, we are almost the only user of it.
  • convert-css-length: a potentially good one to host directly. There are very few lines of code, the problem is solved. Maintained by Gatsby's CEO, I can imagine he has more important things to care about. I thought it was a dependency of Gatsby but it doesn't seem to be so. We are the main users of it: https://npm-stat.com/charts.html?package=convert-css-length (notice the change in May).
  • material-table: we are going with a dedicated Data Grid component.
  • material-pickers: we are further integrating it into the framework :)
  • notistack: this is probably a good one we could integrate back into the framework, long term, but not a priority, it has been doing a great job so far.
  • react-transition-group: I don't think that the current state of transition is awesome, I wonder what we should do about this one.
  • react-virtualized/react-window: it's almost an industry standard? We will see if we can leverage it for the DataGrid, otherwise, we might have to write a dedicated version, or contribute back.
  • popper.js: tricky!
  • material-ui-popup-state: definitely something we should solve in the core, but we never took the time.
  • react-swipeable-views: low ROI, probably not something we want to handle here
  • did I miss another one?

@re-thc
Copy link

re-thc commented Feb 11, 2020

@oliviertassinari react-transition-group should definitely be replaced. Some of the animations feel choppy. Maybe react-spring or similar? Especially on mobile with lower CPU you can feel the difference.

@eps1lon
Copy link
Member

eps1lon commented Feb 11, 2020

It feels safer from our users' perspective.

Feels arguments don't matter. It's indicative of bad education. It's our responsibility to document why we choose certain dependencies.

It gives us the most freedom [...],

I'm not following the freedom argument. The implementation of what dependency would you like to change to fix which bugs/enable what features?

we won't be held back by any of them

We're not that fast. It's hubris to actually believe dependencies hold us back.

react-select [...] downshift

These were not dependencies.

warning

Was removed with an actual rationale beyond "fewer dependencies"

keycode, react-event-listener

Also had a rationale. react-event-listener in particular was obsolete due to better ergonomics with hooks.

We have seen this issue unfold a couple of time, recently we had strict mode [...]

I'm not sure how else to say that because I'm saying it again and again but you keep shifting the blame: That is entirely my fault that it is holding us back at the moment not theirs.

and IE 11 concerns.

And we have IE11 issues in our code. Whether this is caused by a dependency or not is not the cause of IE 11 issues.

So again: What are you achieving by this other than "feeling better"? Objectively this will increase the bundle size for some apps.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Feb 11, 2020

Feels arguments don't matter.

I disagree. Feeling is the single most important level we are working on.

You might enjoy this podcast on a related topic, it's so joyful https://fs.blog/rory-sutherland/ :D.

What's your fear?

Objectively this will increase the bundle size for some apps.

I think that we can ignore this concern for this dependency, it's negligeable.

These were not dependencies.

By using them in our demos, we give them some credit. I think that it starts to blur the line. Our users expect a minimum quality from them.

We're not that fast. It's hubris to actually believe dependencies hold us back.

I think that our mission is probably a more important factor than speed in the "hold back" argument. What problems do we want to own and solve?

That is entirely my fault that it is holding us back at the moment not theirs.

And mine for not using react-jss over a custom @material-ui/styles version that has strict mode issues, no big deal :)

@oliviertassinari oliviertassinari merged commit b841933 into mui:master Feb 12, 2020
@oliviertassinari oliviertassinari deleted the host-normalize-scroll-left branch February 12, 2020 08:52
@oliviertassinari
Copy link
Member Author

@eps1lon
Copy link
Member

eps1lon commented Feb 12, 2020

You might enjoy this podcast on a related topic, it's so joyful fs.blog/rory-sutherland :D.

I'm not listening to a two hour long podcast that is suggested to me laughing at me. If you can't explain the arguments of a two hour long conversations then the ownness is on you.

@oliviertassinari
Copy link
Member Author

@eps1lon the episode is great on its own, outside the discussion we have here, it has changed some of my perspectives when I first listen it :).

EsoterikStare pushed a commit to EsoterikStare/material-ui that referenced this pull request Feb 13, 2020
@mbrookes

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@mbrookes

This comment has been minimized.

@mbrookes

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants