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] Ship modern bundle #22814

Merged
merged 26 commits into from
Oct 12, 2020
Merged

[core] Ship modern bundle #22814

merged 26 commits into from
Oct 12, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Sep 29, 2020

The targets of the default bundle are not final. They will be locked on release of @material-ui/core@5.0.0 from npx browserslist --mobile-to-desktop "> 0.5%, last 2 versions, Firefox ESR, not dead, not IE 11, maintained node versions"

new Supported Platforms documentation
new "minimizing bundle size" guide

We now ship 4 bundles:

  1. stable (default, formerly esm) which targets a snapshot (on release) of > 0.5%, last 2 versions, Firefox ESR, not dead, not IE 11"
  2. modern (formerly es) which targets the last 1 version of evergreen browsers and active node (currently that is 14. We can't use current node since that would include odd-numbered version which is not intended for production code)
  3. node (formerly default) which targets a snapshot (on release) of maintained node versions
  4. legacy which is stable + IE11

/icons only ships node and stable since the other two bundles don't do anything for size but would double the install size which can be problematic (though I haven't been able to reproduce these issues).

I'm not implementing exports and ES modules for node yet. That would require considerably more effort (since we need to import with file extensions) and ES modules are still considered experimental on node (though I think they're planned to be stable in a couple of weeks I seem to recall).

Closes #15496
Closes #18447

/cc @mui-org/core-team For input on the targets (see .browserslistrc).

Future work:

  • Remove internal polyfills required for IE 11 and document it

@mui-pr-bot
Copy link

mui-pr-bot commented Sep 29, 2020

@material-ui/core: parsed: -4.81% 😍, gzip: -5.94% 😍
@material-ui/lab: parsed: -5.25% 😍, gzip: -3.91% 😍
@material-ui/styles: parsed: -1.89% 😍, gzip: -2.18% 😍
@material-ui/system: parsed: -16.16% 😍, gzip: -16.15% 😍
@material-ui/utils: parsed: +8.90% , gzip: +3.46%

Details of bundle changes

Generated by 🚫 dangerJS against d565045

.browserslistrc Outdated
last 1 safari version
node 14

# snapshot of "> 0.5%, last 2 versions, Firefox ESR, not dead, not IE 11" when the last major is released.
Copy link
Member Author

@eps1lon eps1lon Sep 30, 2020

Choose a reason for hiding this comment

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

Widening to 0.2% (which is what create-react-app is using; 0.5 is browserslist default) would include:

  • chrome 49
  • edge 18
  • ios_safari 9.3

Chrome and edge are evergreen so I think we can ignore these versions. Older Chrome versions used to be relevant since it was what google used to crawl sites but they since upgraded and are doing so more often.

Note that safari prior to 12 would need prefixing of gap and column-gap in ImageList. Prior to 10.3 these properties aren't supported at all.

ios_safari cannot be considered evergreen because it's tied to the OS and therefore the device. The devices that support iOS 9 but not iOS 12 are:

  • iPod Touch (5th generation)
  • iPhone 4S
  • iPhone 5
  • iPhone 5C
  • iPad Mini (1st generation)
  • iPad 2
  • iPad (3rd generation)
  • iPad (4th generation)
  • iPad Air
  • iPad Pro (12.9-inch 1st generation)

Using https://en.wikipedia.org/wiki/IOS_9#Supported_devices and https://en.wikipedia.org/wiki/IOS_12#Supported_devices

@eps1lon eps1lon force-pushed the feat/new-bundling branch 2 times, most recently from 99b022e to 9ed4ecb Compare October 5, 2020 10:22
@eps1lon
Copy link
Member Author

eps1lon commented Oct 5, 2020

@material-ui/utils: parsed: +11.22% , gzip: +7.54%

Only due to missing module concatenation (which depends on the app bundle not library code). Stat size is now at 14.58 kB (down from 15.94 kB)

@eps1lon eps1lon force-pushed the feat/new-bundling branch from 9ed4ecb to 4a1eef2 Compare October 5, 2020 11:12
@mbrookes
Copy link
Member

mbrookes commented Oct 12, 2020

I'm not so sure about the [...] and ios_safari change

Me neither. Breaking imagelist on iOS < 12 was unintentional...

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 12, 2020

@mbrookes What do you mean? Only supporting iOS >=13 was intentional.


I have seen the following error in development mode coming from the changes (none of the docs pages are working in dev)

Capture d’écran 2020-10-12 à 18 25 17

I haven't figured out why yet.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 12, 2020

@mbrookes What do you mean? Only supporting iOS >=13 was intentional.

Sure but it's not clear whether this is viable. The benefit of dropping iOS < 13 is that we can use PointerEvent as well.

oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Oct 17, 2020
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Oct 17, 2020
@Jack-Works
Copy link
Contributor

Does it going to ship ES6+ syntax or later? Want to see that happen

@eps1lon
Copy link
Member Author

eps1lon commented Oct 18, 2020

Does it going to ship ES6+ syntax or later? Want to see that happen

The /modern bundle targets the latest 1 versions of major browsers so this should have the least amount of transpilation.

The default bundle should have a lot less transpilation as well.

What is so specific about ES6 syntax that you'd want to see?

@Jack-Works
Copy link
Contributor

Cool, thanks

A second version of the components is also published, which
you can find under the [`/es` folder](https://unpkg.com/@material-ui/core/es/).
All the non-official syntax is transpiled to the [ECMA-262 standard](https://www.ecma-international.org/publications/standards/Ecma-262.htm), nothing more.
⚠️ In order to minimize duplication of code in users' bundles, library authors are **strongly discouraged** to import from any of the other bundles.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for commenting on a closed PR, but is this recommendation saying for libraries to only import either /modern or /legacy? Or is it saying to keep imports as they were before, e.g. @material-ui/core/Button (which would later be used alongside a Babel transform that repoints at either /legacy or /modern, when you build an actual app)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Libraries should not deal with from where they import. This should all be up to the bundler. So yes, import only from @materialui/*/* in your library.

@Izhaki
Copy link
Contributor

Izhaki commented Jun 14, 2021

@oliviertassinari

I repent for using this PR to ask a question, but it's a small one:

Why does MUI distribution includes CommonJS? Won't all React components be bundled anyhow by a bundler that support esm?

What's the use case for a cjs build where a React component will be imported using require?

@Jack-Works
Copy link
Contributor

What's the use case for a cjs build where a React component will be imported using require?

SSR maybe

@eps1lon
Copy link
Member Author

eps1lon commented Jun 15, 2021

The ecosystem isn't ready for ES modules. For example, next.js requires CommonJS.

@Izhaki
Copy link
Contributor

Izhaki commented Jun 15, 2021

For example, next.js requires CommonJS.

Is it? NextJS is using webpack, which will prioritise module over main.

I've got a library of React components and I'm transpiling it with esm only and NextJS is reading it no problem.

I'm asking this question because to the best of my knowledge pretty much all React components are bundled using webpack (for the consuming app at least, not libraries) and webpack has been supporting esm for quite some time.

@eps1lon
Copy link
Member Author

eps1lon commented Jun 15, 2021

Is it?

Yes: vercel/next.js#23725

NextJS is using webpack, which will prioritise module over main.

module and main are only part of native ES modules.

I've got a library of React components and I'm transpiling it with esm only and NextJS is reading it no problem.

Are you sure you're using native ES modules and not some loose interpretation that bundlers used to use? For example, relative file imports do need a file extension. Once you have e.g. import Component from './Component' you no longer have native ES modules.

@Izhaki
Copy link
Contributor

Izhaki commented Jun 15, 2021

Are you sure you're using native ES modules

OK, probably my wrong (am I confusing between esm and es?) I'm not talking about native ES modules (the mjs stuff).

But, if I'm not wrong, the babel config I'm using is borrowed from how MUI used to build the esm folder a year or so ago:

  env: {
    esm: {
      presets: [
        [
          '@babel/preset-env',
          {
            // Do not transform modules - keep as esm
            modules: false,
            targets: {
              node: '6.10',
            },
          },
        ],
      ],
      plugins: [['@babel/plugin-transform-runtime', { useESModules: true }]],
    },
  },

@eps1lon
Copy link
Member Author

eps1lon commented Jun 15, 2021

You're probably shipping something that is only understood by bundlers. With commonJS at least node understands it. With proper ES modules node, browsers and bundlers will understand the code.

@Izhaki

This comment has been minimized.

@eps1lon
Copy link
Member Author

eps1lon commented Jun 15, 2021

You never execute these on node, or import them without some bundler being involved?

You do when doing server-side rendering.

@Izhaki
Copy link
Contributor

Izhaki commented Jun 15, 2021

You do when doing server-side rendering.

But this is done via webpack, which goes to the "module" field in package.json and reads files distributed per the 4.1.2 config above alright.

NextJS uses webpack.
createReactApp uses webpack.
Is anybody out there builds React apps not via webpack or some other bundler that understand that sort of esm export like in 4.1.2?

@Izhaki
Copy link
Contributor

Izhaki commented Jun 15, 2021

OK. I think I see what I'm missing. I've been with NextJS for too long probably...

Some people (as in some React examples) do SSR where the server simply requires the React Component (no webpack involved). These people are unlikely to write cjs style - most likely they write es6 and have some transpiler for the code, but not for node_modules.

So I guess the answer is - for people who do SSR without a bundler.

@Jack-Works
Copy link
Contributor

Yes, the most simple SSR is to use react-dom/server package to render the JSX directly. That requires the code to be able to run in NodeJS directly.

presets: defaultPresets.concat(['@babel/preset-react', '@babel/preset-typescript']),
plugins: [
module.exports = function getBabelConfig(api) {
const useESModules = api.env(['legacy', 'modern', 'stable']);

Choose a reason for hiding this comment

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

Am I reading this right that legacy SHOULD use ESModules? This is almost certainly a mistake, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this stands for legacy ESM.

From microbundle readme:

	"module": "./dist/foo.module.js", // legacy ES Modules output (for bundlers)
	"exports": "./dist/foo.modern.js", // modern ES2017 output

Copy link
Member Author

Choose a reason for hiding this comment

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

Am I reading this right that legacy SHOULD use ESModules? This is almost certainly a mistake, right?

Could you open an issue and fill out the template to describe the mistake? Discussions on merged PRs are oftentimes forgotten.

Choose a reason for hiding this comment

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

I guess I thought legacy was for ancient browsers like ie11, it seems that I might be incorrect about that. Thanks for the explanation, sorry for the mis-informed question. Have a nice day.

Copy link
Member Author

Choose a reason for hiding this comment

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

legacy is for IE11. But IE11 does not understand ES modules nor CommonJS modules i.e. the code always needs a bundler anyway at which point we might as well ship ES modules to help tree-shaking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Bundle plans [RFC] Support only last n browser versions
9 participants