[DatePicker] Use only path imports from @material-ui/core

[oliviertassinari]

Material-UI forbid imports over two levels as it can lead to duplication in people bundles. Meaning, if somebody does:

import { Modal } from '@material-ui/core'; // 1.
import TrapFocus from '@material-ui/core/Modal/TrapFocus'; // 2.

It will bundle 1. and 2.:

1. https://unpkg.com/browse/@material-ui/core@4.9.10/esm/Modal/TrapFocus.js
2. https://unpkg.com/browse/@material-ui/core@4.9.10/Modal/TrapFocus.js

Now, given we do:

https://github.com/mui-org/material-ui-pickers/blob/7bed283699ef768936a3ec7c5dc89571492dddd4/lib/src/wrappers/DesktopPopperWrapper.tsx#L6

in the codebase, we very likely cause this double bundling quite often. For instance:

import { Dialog } from '@material-ui/core';
import { DateRangePicker } from '@material-ui/pickers';

[dmtrKovalenko]

Hmm, I am not really sure about why? In theory, webpack should understand that we are importing the same file.

P.S. I'd like not use @material-ui/core/Button imports because of typescript autoimport. I am not writing imports manually at all. Is there any preference of using path imports instead of index?

By the way, is there any eslint plugin that will automatically change the import. If no – I can write one

[TrySound]

@dmtrKovalenko @material-ui/core/Modal/TrapFocus.js and @material-ui/core/esm/Modal/TrapFocus.js is not the same file

[dmtrKovalenko]

ohh god, really... thanks @TrySound then its definitely a problem
Moreover, I think we need to provide more clear esm build.

[oliviertassinari]

It can be verified with this sandbox https://codesandbox.io/s/wonderful-yonath-5cted?file=/src/App.js I have deployed for production in https://csb-5cted.netlify.com/.

Duplication 🔽

[TrySound]

unstable_TrapFocus export is a good way to go for internal usage.

[dmtrKovalenko]

I mean overall. Does import from the core for the current build is somehow worse than path import?
I`d like to open another issue to clarify imports

[oliviertassinari]

By the way, is there any eslint plugin that will automatically change the import. If no – I can write one

We have this eslint plugin for internal usage of the mono-repository: https://github.com/mui-org/material-ui/blob/master/packages/eslint-plugin-material-ui, It could be great to publish it on npm for external users. If this is something you want to work on 👍.
For the date picker components, there is no much need to worry about it, we will get it for free with a merge of the git repository.

I am not really sure about why?

Regarding, the why. Let's take my repository and expand the resolution, using equivalences:

import { Dialog } from '@material-ui/core';
import { DateRangePicker } from '@material-ui/pickers';

<=>

import Dialog from '@material-ui/core/esm/Dialog.js';
import TrapFocus from '@material-ui/core/Modal/TrapFocus';

<=>

import Modal from '@material-ui/core/esm/Modal.js';
https://unpkg.com/browse/@material-ui/core@4.9.10/Modal/TrapFocus.js

<=>

https://unpkg.com/browse/@material-ui/core@4.9.10/esm/Modal/TrapFocus.js
https://unpkg.com/browse/@material-ui/core@4.9.10/Modal/TrapFocus.js

[dmtrKovalenko]

I figured out why. But another question of why do we need an esm folder – it is confusing and easily can lead to duplicates in developers bundle. And do we need path imports at all, if core exports are treeshakable.

[oliviertassinari]

Ok great

why do we need an esm folder? Do we need path imports at all?

It explained in https://material-ui.com/guides/minimizing-bundle-size/. Adding @eps1lon to the loop.

[dmtrKovalenko]

⚠️ The following instructions are only needed if you want to optimize your development startup times or if you are using an older bundler that doesn't support tree-shaking.

So I suppose we need to just use core imports on core documentation overall.

[eps1lon]

Allowing certain path imports by convention is definitely a brittle approach. In the future we likely leverage export maps so that it's obvious what's allowed and what isn't.

But another question of why do we need an esm folder

Because it would require rewriting file extensions instead. We use relative imports without extensions throughout the codebase and it's not clear to me that all bundlers (during v4 development) were capable of figuring out which files is meant when importing './TrapFocus': TrapFocus.mjs or TrapFocus.js.

The goal is definitely to publish a correct ES modules build at some point but we need to know if this works across bundlers and need to implement this as well. The /esm folder is a simple solution that is used throughout the ecosystem. In hindsight we should've moved all source files into /dist to make it obvious that you're in dangerous territory.

Either way

unstable_TrapFocus export is a good way to go for internal usage.

seems like good tradeoff. We definitely don't want to add more exceptions to what kind of path imports are allowed.

So I suppose we need to just use core imports on core documentation overall.

Not really following what you mean. We shouldn't use path imports deeper than @material-ui/core/FirstLevelModuel in the documentation. If you find deeper imports please let us know.

[dmtrKovalenko]

@eps1lon thanks for description. I am not 100% sure why we cannot use@material-ui/core imports now? Or at least make it the recommended way to import? Is there some performance issue, because they are properly treeshaking by all bundlers as for now.

[eps1lon]

I am not 100% sure why we cannot use@material-ui/core imports now?

Tree-shaking is by default a production-only feature in webpack 4 and earlier (not sure about 5). This means that webpack will parse the full /core module even if you only import a single part of it. This means that you have a slower cold-start of your webpack dev server. By importing from /core/Button webpack will only parse /core/Button and its imports which is far less than the full module.

It doesn't affect user code at all. It's a dev-only optimization. I haven't followed the discussion around this so this might've changed in new versions of webpack.

[dmtrKovalenko]

Thanks, got it @eps1lon
Ok, so we need to actually change all the imports for pickers. Only one pain in the ass left – is auto imports.

For now, I`ll just copy and enhance the rule from the https://github.com/mui-org/material-ui/tree/master/packages/eslint-plugin-material-ui

[eps1lon]

Ok, so we need to actually change all the imports for pickers. Only one pain in the ass left – is auto imports.

We should probably bite the bullet and publish a babel plugin for that. Having different approaches floating around in "userspace" is pretty dangerous. The only problem is how we make it compatible with any (minor v4) version of Material-UI.

[NMinhNguyen]

It could be great to publish it on npm for external users. If this is something you want to work on 👍.

I ended up needing it recently so I just copied and pasted it. But I was wondering what's left to do and why it can't just be published? Seems to be working fine? Sorry if this isn't the right repo to discuss.

[oliviertassinari]

@mbrookes Could you extend the number of people that have the publish rights on https://www.npmjs.com/package/eslint-plugin-material-ui? I think that it could be interesting to publish a new version, at least for internal usage.

[mbrookes]

😱(Has it been that long?!)

Could you extend the number of people that have the publish rights

Done.

[oliviertassinari]

@mbrookes Thanks, yeah wow.

[mbrookes]

I can't now remember why it was published in the first place, considering it's only used internally... 🤔