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] Fix transpilation target of UMD bundle #23618

Merged
merged 1 commit into from Nov 20, 2020
Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Nov 19, 2020

  1. UMD bundle is not transpiled with the proper target (from .browserslistrc)
  2. yarn test:karma issues warnings about imports not being resolved
    This is not severe since tests pass fine but is noisy

This was introduced in 4b71794#diff-09c56b2bf95de2a608a36afef3b6893146a959d6739be0a154dc9c9f02d80f24L7-R20 in order to let yarn workspace @material-ui/core build:umd pass. However, we can use existing patterns (use named imports over path imports) without having to change infra. Changing build infra is always scary since we don't (and likely never have) a proper coverage for that output.

@eps1lon eps1lon added the package: material-ui Specific to @mui/material label Nov 19, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Nov 19, 2020

Details of bundle changes

Generated by 🚫 dangerJS against eaef85c

@eps1lon eps1lon marked this pull request as ready for review November 19, 2020 16:43
SliderValueLabelUnstyled,
sliderClasses,
} from '@material-ui/unstyled/SliderUnstyled';
import { SliderUnstyled, SliderValueLabelUnstyled, sliderClasses } from '@material-ui/unstyled';
Copy link
Member

Choose a reason for hiding this comment

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

The motivation was to allow:

import Slider from '@material-ui/core/Slider';

to be an effective way to tree-shake the bundle, even in development. If we move in this direction, I think that we might as well update the demos. We would load the whole unstyled package. it seems to yield the same properties with a nicer DX:

import { Slider } from '@material-ui/core';

I think that we should be careful with how it will scale. We can look at the icon for that.
Next.js renders the following page in less than 1s ✅.

import React from 'react';
import CloseIcon from '@material-ui/icons/Close';

export default function LandingPage() {
  return (
    <div>
      <CloseIcon />
    </div>
  );
}

However, If you take this demo, it takes ~60s for Next.js to render it ❌.

import React from 'react';
import { Close as CloseIcon } from '@material-ui/icons';

export default function LandingPage() {
  return (
    <div>
      <CloseIcon />
    </div>
  );
}

Copy link
Member Author

@eps1lon eps1lon Nov 20, 2020

Choose a reason for hiding this comment

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

Let's focus on the concrete case not icons and deal with scaling issues after we actually scaled. See "YAGNI" and "don't optimize before you meassure".

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, icons was probably not the best comparison I could have picked.

a.

import Alert from '@material-ui/core/Alert';

vs.

b.

import { Alert } from '@material-ui/core';

would have been a to more accurate considering that unstyled will have about the same amount of modules as core. It turns out that there isn't much difference in this Alert case (a. 1.9s vs. b. 6.22s), I mean, below 10s could be good enough.

I guess I was more concerned about what would happen if we x2 the number of components in the core. Would we start to hit a slowness?

Copy link
Member

Choose a reason for hiding this comment

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

@eps1lon Is this measurement better 🔼 :). Do you agree with the hypothesis that below 10s we are good and above it starts to be an issue (relative to the machine used)?

@eps1lon
Copy link
Member Author

eps1lon commented Nov 20, 2020

Let's get next in a good shape and then deal with reproducible issues later.

@eps1lon eps1lon merged commit 2be9e97 into mui:next Nov 20, 2020
@eps1lon eps1lon deleted the fix/umd-target branch November 20, 2020 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants