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] Migrate to import * as React from 'react' #19802

Merged
merged 7 commits into from
Feb 26, 2020

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Feb 22, 2020

@mui-pr-bot
Copy link

mui-pr-bot commented Feb 22, 2020

@material-ui/core: parsed: -0.79% 😍, gzip: -0.17% 😍
@material-ui/lab: parsed: -0.53% 😍, gzip: -0.11% 😍

Details of bundle changes.

Comparing: 4c396bd...4854cde

bundle Size Change Size Gzip Change Gzip
@material-ui/core ▼ -2.85 kB (-0.79% ) 359 kB ▼ -165 B (-0.17% ) 98.9 kB
docs.main ▼ -1.37 kB (-0.23% ) 601 kB ▼ -96 B (-0.05% ) 195 kB
@material-ui/lab ▼ -1.03 kB (-0.53% ) 193 kB ▼ -65 B (-0.11% ) 57.3 kB
TablePagination ▼ -963 B (-0.67% ) 143 kB ▼ -68 B (-0.16% ) 42 kB
TextField ▼ -742 B (-0.59% ) 125 kB ▲ +16 B (+0.04% ) 36.7 kB
Select ▼ -634 B (-0.54% ) 116 kB ▲ +3 B (+0.01% ) 34.7 kB
Autocomplete ▼ -470 B (-0.35% ) 132 kB ▼ -5 B (-0.01% ) 41.5 kB
SpeedDialAction ▼ -363 B (-0.31% ) 118 kB ▲ +14 B (+0.04% ) 37.6 kB
SwipeableDrawer ▼ -333 B (-0.36% ) 92.3 kB ▼ -12 B (-0.04% ) 28.9 kB
Menu ▼ -332 B (-0.37% ) 88.8 kB ▲ +17 B (+0.06% ) 27.5 kB
Radio ▼ -298 B (-0.35% ) 84.1 kB ▲ +7 B (+0.03% ) 26.6 kB
Tabs ▼ -285 B (-0.33% ) 85.5 kB ▼ -2 B (-0.01% ) 27.2 kB
Checkbox ▼ -278 B (-0.33% ) 83.1 kB ▲ +1 B (0.00% ) 26.3 kB
StepButton ▼ -267 B (-0.32% ) 82.5 kB ▼ -4 B (-0.02% ) 26.2 kB
Alert ▼ -260 B (-0.31% ) 83.6 kB ▲ +18 B (+0.07% ) 26.4 kB
Drawer ▼ -248 B (-0.29% ) 84.9 kB ▲ +32 B (+0.12% ) 25.9 kB
Pagination ▼ -245 B (-0.29% ) 85.3 kB ▲ +24 B (+0.09% ) 26.3 kB
Switch ▼ -238 B (-0.29% ) 82.3 kB -- 26 kB
SpeedDial ▼ -234 B (-0.27% ) 86.4 kB ▲ +7 B (+0.03% ) 27.3 kB
Breadcrumbs ▼ -226 B (-0.28% ) 80.6 kB ▼ -10 B (-0.04% ) 25.4 kB
Popover ▼ -226 B (-0.27% ) 83.2 kB ▲ +10 B (+0.04% ) 25.8 kB
MenuItem ▼ -225 B (-0.29% ) 78.4 kB ▲ +14 B (+0.06% ) 24.6 kB
Chip ▼ -213 B (-0.26% ) 82.8 kB ▲ +14 B (+0.06% ) 25.4 kB
ListItem ▼ -212 B (-0.27% ) 77.3 kB ▲ +12 B (+0.05% ) 24.3 kB
PaginationItem ▼ -211 B (-0.26% ) 81 kB ▲ +22 B (+0.09% ) 25 kB
Tooltip ▼ -210 B (-0.20% ) 102 kB ▲ +7 B (+0.02% ) 32.4 kB
Dialog ▼ -208 B (-0.25% ) 83.2 kB ▼ -17 B (-0.07% ) 25.9 kB
NativeSelect ▼ -199 B (-0.26% ) 77.1 kB ▼ -4 B (-0.02% ) 24.4 kB
TableSortLabel ▼ -197 B (-0.25% ) 77.6 kB ▲ +16 B (+0.07% ) 24.4 kB
ExpansionPanelSummary ▼ -193 B (-0.25% ) 78.3 kB -- 24.8 kB
ButtonGroup ▼ -188 B (-0.22% ) 83.4 kB ▲ +27 B (+0.11% ) 25.6 kB
Button ▼ -169 B (-0.21% ) 80 kB ▲ +20 B (+0.08% ) 24.6 kB
BottomNavigationAction ▼ -167 B (-0.22% ) 75.8 kB ▲ +6 B (+0.03% ) 24 kB
IconButton ▼ -165 B (-0.22% ) 76.4 kB ▲ +28 B (+0.12% ) 23.9 kB
Fab ▼ -165 B (-0.21% ) 77.1 kB ▲ +25 B (+0.10% ) 24.1 kB
CardActionArea ▼ -165 B (-0.22% ) 75.3 kB ▲ +13 B (+0.05% ) 23.8 kB
Tab ▼ -165 B (-0.22% ) 76.6 kB ▼ -2 B (-0.01% ) 24.3 kB
ToggleButton ▼ -165 B (-0.22% ) 76.4 kB ▲ +1 B (0.00% ) 24.2 kB
Snackbar ▼ -152 B (-0.20% ) 75.6 kB ▼ -6 B (-0.03% ) 23.7 kB
ButtonBase ▼ -150 B (-0.20% ) 74.3 kB ▲ +25 B (+0.11% ) 23.4 kB
Modal ▼ -144 B (-0.99% ) 14.3 kB ▼ -6 B (-0.12% ) 5.03 kB
OutlinedInput ▼ -131 B (-0.17% ) 74.9 kB ▲ +8 B (+0.03% ) 23.4 kB
Rating ▼ -122 B (-0.17% ) 70.7 kB ▼ -13 B (-0.06% ) 22.7 kB
Slider ▼ -120 B (-0.16% ) 76.9 kB ▼ -14 B (-0.06% ) 24.2 kB
docs.landing ▼ -114 B (-0.20% ) 56.6 kB ▼ -6 B (-0.04% ) 15.6 kB
FilledInput ▼ -113 B (-0.15% ) 73.9 kB ▲ +17 B (+0.07% ) 23 kB
Input ▼ -113 B (-0.15% ) 72.9 kB ▲ +16 B (+0.07% ) 22.8 kB
InputBase ▼ -100 B (-0.14% ) 71 kB ▲ +14 B (+0.06% ) 22.3 kB
TreeItem ▼ -95 B (-0.13% ) 74.3 kB ▲ +4 B (+0.02% ) 23.5 kB
Popper ▼ -90 B (-0.31% ) 28.8 kB ▲ +11 B (+0.11% ) 10.3 kB
StepLabel ▼ -89 B (-0.13% ) 68.9 kB ▼ -12 B (-0.06% ) 21.7 kB
ExpansionPanel ▼ -87 B (-0.12% ) 72.6 kB ▼ -9 B (-0.04% ) 22.7 kB
MenuList ▼ -79 B (-0.12% ) 66.3 kB ▲ +9 B (+0.04% ) 20.8 kB
useAutocomplete ▼ -77 B (-0.52% ) 14.7 kB ▼ -12 B (-0.22% ) 5.33 kB
RadioGroup ▼ -76 B (-0.12% ) 64.7 kB ▲ +28 B (+0.14% ) 20.1 kB
ClickAwayListener ▼ -72 B (-1.84% ) 3.84 kB ▼ -14 B (-0.90% ) 1.54 kB
FormControl ▼ -64 B (-0.10% ) 64.7 kB ▲ +6 B (+0.03% ) 20.2 kB
Link ▼ -61 B (-0.09% ) 66.9 kB ▼ -15 B (-0.07% ) 21.1 kB
TreeView ▼ -57 B (-0.09% ) 67 kB ▼ -7 B (-0.03% ) 21.1 kB
MobileStepper ▼ -55 B (-0.08% ) 68.1 kB ▼ -27 B (-0.13% ) 21.4 kB
StepIcon ▼ -55 B (-0.08% ) 64.9 kB ▼ -8 B (-0.04% ) 20.3 kB
FormControlLabel ▼ -54 B (-0.08% ) 65.8 kB ▲ +4 B (+0.02% ) 20.7 kB
Stepper ▼ -53 B (-0.08% ) 65.2 kB ▼ -11 B (-0.05% ) 20.6 kB
Avatar ▼ -53 B (-0.08% ) 65.5 kB ▲ +1 B (0.00% ) 20.7 kB
InputLabel ▼ -52 B (-0.08% ) 65.6 kB ▼ -1 B (-0.00% ) 20.5 kB
SpeedDialIcon ▼ -51 B (-0.08% ) 64.9 kB ▼ -7 B (-0.03% ) 20.3 kB
TextareaAutosize ▼ -50 B (-0.95% ) 5.19 kB ▼ -18 B (-0.82% ) 2.17 kB
Portal ▼ -47 B (-1.61% ) 2.87 kB ▼ -14 B (-1.07% ) 1.29 kB
Slide ▼ -47 B (-0.18% ) 25.6 kB ▼ -2 B (-0.02% ) 8.74 kB
Hidden ▼ -45 B (-0.07% ) 66.3 kB ▼ -12 B (-0.06% ) 20.8 kB
InputAdornment ▼ -43 B (-0.07% ) 65.4 kB ▲ +11 B (+0.05% ) 20.6 kB
StepContent ▼ -42 B (-0.06% ) 69.5 kB ▼ -6 B (-0.03% ) 21.8 kB
Backdrop ▼ -41 B (-0.06% ) 68.2 kB ▲ +12 B (+0.06% ) 21.1 kB
FormLabel ▼ -39 B (-0.06% ) 63.8 kB ▲ +9 B (+0.05% ) 19.8 kB
FormHelperText ▼ -39 B (-0.06% ) 63.7 kB ▼ -5 B (-0.02% ) 20 kB
ListItemText ▼ -39 B (-0.06% ) 65.3 kB ▲ +5 B (+0.02% ) 20.5 kB
GridListTile ▼ -38 B (-0.06% ) 64 kB ▼ -2 B (-0.01% ) 20.1 kB
CardHeader ▼ -36 B (-0.06% ) 65.4 kB ▲ +5 B (+0.02% ) 20.6 kB
Grow ▼ -32 B (-0.13% ) 24.2 kB ▼ -2 B (-0.02% ) 8.22 kB
TableCell ▼ -31 B (-0.05% ) 64.3 kB ▲ +1 B (0.00% ) 20.3 kB
SnackbarContent ▼ -30 B (-0.05% ) 63.8 kB ▼ -12 B (-0.06% ) 20.1 kB
DialogTitle ▼ -28 B (-0.04% ) 64.6 kB -- 20.3 kB
Collapse ▼ -27 B (-0.04% ) 68.3 kB -- 21.2 kB
ScopedCssBaseline ▼ -26 B (-0.04% ) 63.1 kB ▼ -12 B (-0.06% ) 19.9 kB
DialogContentText ▼ -26 B (-0.04% ) 64.4 kB ▼ -8 B (-0.04% ) 20.2 kB
Card ▼ -26 B (-0.04% ) 63.2 kB ▲ +6 B (+0.03% ) 19.8 kB
Zoom ▼ -26 B (-0.11% ) 23.6 kB ▲ +6 B (+0.07% ) 8.13 kB
AlertTitle ▼ -26 B (-0.04% ) 64.5 kB ▼ -4 B (-0.02% ) 20.3 kB
AppBar ▼ -26 B (-0.04% ) 64.3 kB ▲ +2 B (+0.01% ) 20.2 kB
Fade ▼ -26 B (-0.11% ) 23.6 kB ▼ -1 B (-0.01% ) 8.01 kB
RootRef ▼ -24 B (-0.57% ) 4.21 kB ▼ -14 B (-0.85% ) 1.63 kB
List ▼ -24 B (-0.04% ) 62.7 kB ▲ +8 B (+0.04% ) 19.6 kB
Table ▼ -24 B (-0.04% ) 62.9 kB -- 19.7 kB
ListItemAvatar ▼ -22 B (-0.04% ) 62.4 kB ▲ +3 B (+0.02% ) 19.6 kB
ListItemIcon ▼ -22 B (-0.04% ) 62.5 kB ▲ +2 B (+0.01% ) 19.6 kB
TableBody ▼ -22 B (-0.04% ) 62.4 kB ▼ -2 B (-0.01% ) 19.5 kB
TableFooter ▼ -22 B (-0.04% ) 62.4 kB ▼ -2 B (-0.01% ) 19.5 kB
TableHead ▼ -22 B (-0.04% ) 62.4 kB ▼ -2 B (-0.01% ) 19.5 kB
TableRow ▼ -22 B (-0.04% ) 62.8 kB ▲ +2 B (+0.01% ) 19.7 kB
NoSsr ▼ -21 B (-0.96% ) 2.17 kB ▼ -7 B (-0.67% ) 1.03 kB
Step ▼ -21 B (-0.03% ) 63 kB ▼ -4 B (-0.02% ) 19.8 kB
GridListTileBar ▼ -21 B (-0.03% ) 63.5 kB ▼ -2 B (-0.01% ) 19.9 kB
LinearProgress ▼ -19 B (-0.03% ) 65.6 kB ▼ -14 B (-0.07% ) 20.5 kB
AvatarGroup ▼ -19 B (-0.03% ) 62.7 kB ▼ -10 B (-0.05% ) 19.7 kB
GridList ▼ -19 B (-0.03% ) 62.8 kB ▼ -4 B (-0.02% ) 19.7 kB
BottomNavigation ▼ -19 B (-0.03% ) 62.7 kB ▼ -3 B (-0.02% ) 19.7 kB
ToggleButtonGroup ▼ -19 B (-0.03% ) 63.5 kB ▼ -2 B (-0.01% ) 20 kB
@material-ui/core[umd] ▼ -18 B (-0.01% ) 318 kB ▲ +34 B (+0.04% ) 92.2 kB
CircularProgress ▼ -17 B (-0.03% ) 64.4 kB -- 20.3 kB
StepConnector ▼ -15 B (-0.02% ) 63 kB ▼ -12 B (-0.06% ) 19.8 kB
SvgIcon ▼ -15 B (-0.02% ) 63.4 kB ▲ +6 B (+0.03% ) 19.8 kB
Badge ▼ -15 B (-0.02% ) 65.6 kB ▲ +2 B (+0.01% ) 20.4 kB
Toolbar ▼ -13 B (-0.02% ) 62.7 kB ▼ -16 B (-0.08% ) 19.7 kB
FormGroup ▼ -13 B (-0.02% ) 62.3 kB ▼ -14 B (-0.07% ) 19.5 kB
Skeleton ▼ -13 B (-0.02% ) 63.3 kB ▼ -11 B (-0.05% ) 20 kB
CssBaseline ▼ -13 B (-0.02% ) 62.3 kB ▼ -10 B (-0.05% ) 19.6 kB
ListSubheader ▼ -13 B (-0.02% ) 63.1 kB ▼ -10 B (-0.05% ) 19.8 kB
useMediaQuery ▼ -13 B (-0.50% ) 2.56 kB ▼ -7 B (-0.66% ) 1.06 kB
Icon ▼ -13 B (-0.02% ) 63.1 kB ▼ -6 B (-0.03% ) 19.8 kB
Typography ▼ -13 B (-0.02% ) 64 kB ▲ +5 B (+0.02% ) 20 kB
CardActions ▼ -13 B (-0.02% ) 62.4 kB ▲ +3 B (+0.02% ) 19.6 kB
CardMedia ▼ -13 B (-0.02% ) 62.7 kB ▼ -3 B (-0.02% ) 19.7 kB
CardContent ▼ -13 B (-0.02% ) 62.3 kB ▼ -2 B (-0.01% ) 19.5 kB
DialogContent ▼ -13 B (-0.02% ) 62.5 kB ▼ -2 B (-0.01% ) 19.6 kB
Grid ▼ -13 B (-0.02% ) 65.4 kB ▲ +2 B (+0.01% ) 20.5 kB
ListItemSecondaryAction ▼ -13 B (-0.02% ) 62.3 kB ▼ -2 B (-0.01% ) 19.5 kB
TableContainer ▼ -13 B (-0.02% ) 62.3 kB ▼ -2 B (-0.01% ) 19.5 kB
Container ▼ -13 B (-0.02% ) 63.5 kB ▲ +1 B (+0.01% ) 19.9 kB
Divider ▼ -13 B (-0.02% ) 63 kB ▲ +1 B (+0.01% ) 19.8 kB
ExpansionPanelActions ▼ -13 B (-0.02% ) 62.4 kB ▼ -1 B (-0.01% ) 19.6 kB
ExpansionPanelDetails ▼ -13 B (-0.02% ) 62.3 kB ▼ -1 B (-0.01% ) 19.5 kB
Paper ▼ -13 B (-0.02% ) 62.7 kB ▲ +1 B (+0.01% ) 19.6 kB
DialogActions ▼ -13 B (-0.02% ) 62.4 kB -- 19.6 kB
@material-ui/styles -- 51.4 kB -- 15.4 kB
@material-ui/system -- 16.5 kB -- 4.32 kB
Box -- 72.2 kB -- 21.9 kB
colorManipulator -- 3.88 kB -- 1.52 kB
styles/createMuiTheme -- 16.6 kB -- 5.85 kB

Generated by 🚫 dangerJS against 4854cde

@oliviertassinari
Copy link
Member

What about jaredpalmer/tsdx#152 (comment)?

@TrySound
Copy link
Contributor Author

TrySound commented Feb 22, 2020

React team did decision here. It's not an open question anymore. This may kinda break rollup though I think it's on user to provide all possible exports. Can be solved by this

import * as react from 'react';
import * as reactDom from 'react-dom';
import * as reactIs from 'react-is';
import * as propTypes from 'prop-types';
...
    commonjs({
      namedExports: {
        react: Object.keys(react),
        'react-dom': Object.keys(reactDom),
        'react-is': Object.keys(reactIs),
        'prop-types': Object.keys(propTypes),
      },
    }),

Also even this is gonna be fixed by rollup soon
rollup/plugins#149

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Very nice. We can merge once CI is green.

@eps1lon eps1lon added the core Infrastructure work going on behind the scenes label Feb 22, 2020
@eps1lon eps1lon changed the title Migrate to import * as React from 'react' [core] Migrate to import * as React from 'react' Feb 22, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 23, 2020

I don't have much context on the difference, I would be curious to hear more about the following:

  1. Has react updated the documentation to reflect the change?
  2. Has react released the changes yet?
  3. Has react introduced a warning for default imports yet?
  4. What about this diff?
-import React, {Fragment, useCallback, useState} from 'react';
+import * as React from 'react';
+import {Fragment, useCallback, useState} from 'react';

I think that it would be interesting to delay things. We could see how the other React packages approach the problem or keep the changes internal (leaving the documentation outside of it).

What do you think?

@TrySound
Copy link
Contributor Author

  1. Nope, but it will. We know the decision.
  2. It was small code optimisation. Nothing is changed or broken.
  3. Not yet so you have a time to migrate and prevent complains from users.
  4. Do you want to write React.useState? I can change if you want. I write this way all the time.

Some packages denied import React long time ago. I saw complains only from rollup users.
https://github.com/realadvisor/rifm/blob/master/src/Rifm.js#L3
https://github.com/jaredpalmer/formik/blob/master/packages/formik/src/Formik.tsx#L1
https://github.com/tizmagik/react-head/blob/master/src/HeadTag.js#L1

@TrySound
Copy link
Contributor Author

@eps1lon
Copy link
Member

eps1lon commented Feb 23, 2020

As far as I know import React from 'react' was never correct. It just happened to work because bundlers/transpilers managed commonJS and ESModule interopability issues.

So yeah we should stick with import * as React instead of import React.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 23, 2020

  1. https://twitter.com/sebmarkbage/status/1231673639080038400 puts some context on the motivation for the change. Given https://twitter.com/sebmarkbage/status/1231674025547423744 I wonder if it will ever happen.
  2. Ok, so it shouldn't have any downstream impact, at the rare exception to developers that build their app with a wrong rollup configuration? It sounds great.
  3. Ok, so we don't need to rush the change.
  4. I was wondering about the diff. I personally think that we should stick to React.x as long as possible in the documentation. Not having to do back and forths between the usage of a method from React and the imports at the top of the file is a time saver. It's more ergonomic.

@eps1lon @TrySound Maybe we could employ two different strategies, import * as React from for the internals that uses ESM (gain 0.2% of bundle size), and import React for anything user-facing, documentation, examples, demos?

@oliviertassinari
Copy link
Member

@TrySound Thanks for the update. Should we apply the same change to the other packages (icons, lab, etc.)?

@TrySound
Copy link
Contributor Author

That would be good. I opened this PR mostly as demo.

@eps1lon
Copy link
Member

eps1lon commented Feb 25, 2020

Maybe we could employ two different strategies, import * as React from for the internals that uses ESM (gain 0.2% of bundle size), and import React for anything user-facing, documentation, examples, demos?

Fine by me. Though it would help with the demos since we don't need esModuleInterop for TS demos.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 25, 2020

Fine by me. Though it would help with the demos since we don't need esModuleInterop for TS demos.

As long as the demos match what most the developers are using in their applications, this sounds great. My concern is about not using Material-UI to influence the community both rather to follow it. In the case of JavaScript, I have very rarely seen import * as React used.
However, could it be more common in TypeScript? for instance https://www.typescriptlang.org/docs/handbook/react-&-webpack.html.

@eps1lon
Copy link
Member

eps1lon commented Feb 25, 2020

My concern is about not using Material-UI to influence the community both rather to follow it.

This sounds out of character. I was under the impression you wanted to do the opposite. Maybe I misinterpreted past statements of yours.

@oliviertassinari
Copy link
Member

@eps1lon I think that Material-UI's objective should be providing what developers truly want (e.g. not what they say us they need). At the end of the day, it's all about how they feel about it.
If we don't provide what developers want, another product will. Does it match?

@eps1lon
Copy link
Member

eps1lon commented Feb 26, 2020

I think that Material-UI's objective should be providing what developers truly want

So we do want to influence them. If we want to follow then we do what we're told. If we want to lead we're doing what we think they want.

What you're describing is incoherent.

@eps1lon eps1lon merged commit 4fba0da into mui:master Feb 26, 2020
@TrySound TrySound deleted the react-namespace branch February 26, 2020 08:25
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 26, 2020

Ok, per these terms definitions, it's incoherent.

I think that we should be on the "leading" side of it. This means using empathy with the users, identifying their biggest problems, finding the root issues, finding solutions, implementing them, making sure the solution is technically solid, finally and more importantly, selling that our solution is great, create excitement.

I would like to add the following note. There is a wide gap between what people say they will do and what they actually do. Actions are significantly more meaningful. In the previous message, when I said "what people truly want", I was referring to their future actions.

EsoterikStare pushed a commit to EsoterikStare/material-ui that referenced this pull request Mar 30, 2020
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.

4 participants