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

[Pagination] v5 usePagination requires @emotion #27282

Closed
2 tasks done
MichielDeMey opened this issue Jul 14, 2021 · 5 comments · Fixed by #27348
Closed
2 tasks done

[Pagination] v5 usePagination requires @emotion #27282

MichielDeMey opened this issue Jul 14, 2021 · 5 comments · Fixed by #27348
Labels
component: pagination This is the name of the generic UI component, not the React module! new feature New feature or request package: base-ui Specific to @mui/base ready to take Help wanted. Guidance available. There is a high chance the change will be accepted v5.x migration

Comments

@MichielDeMey
Copy link

When simply using the usePagination hook provided by MaterialUI, @emotion/styled and @emotion/react are mandatory dependencies, otherwise it will throw a Webpack build error:

Failed to compile.

./node_modules/@material-ui/styled-engine/index.js
Module not found: Can't resolve '@emotion/react' in '/Users/username/Documents/project/node_modules/@material-ui/styled-engine'
  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Does not compile unless @emotion packages are installed

Expected Behavior 🤔

I would expect to be able to compile without an exception since no styles are actually used or needed when using the hook.

Steps to Reproduce 🕹

Codesandbox to reproduce the isse
https://codesandbox.io/s/fluentui-use-paginate-7czwv?file=/src/components/Paginate/index.tsx

Steps:

  1. Remove the @emotion packages
  2. Compilation exception

Context 🔦

We're trying to use the new headless components (hooks) provided by MaterialUI, but without any styling requirements.

Your Environment 🌎

`npx @material-ui/envinfo`
System:
    OS: macOS 11.4
  Binaries:
    Node: 16.4.2 - ~/.n/bin/node
    Yarn: 1.22.10 - ~/.n/bin/yarn
    npm: 7.18.1 - ~/.n/bin/npm
  Browsers:
    Chrome: 91.0.4472.114
    Edge: Not Found
    Firefox: Not Found
    Safari: 14.1.1
  npmPackages:
    @material-ui/core: ^5.0.0-beta.0 => 5.0.0-beta.0
    @material-ui/private-theming:  5.0.0-beta.0
    @material-ui/styled-engine:  5.0.0-beta.0
    @material-ui/system:  5.0.0-beta.0
    @material-ui/types:  6.0.1
    @material-ui/unstyled:  5.0.0-alpha.39
    @material-ui/utils:  5.0.0-beta.0
    @types/react: ^16.9.0 => 16.14.11
    react: ^16.13.1 => 16.14.0
    react-dom: ^16.13.1 => 16.14.0
    typescript: ^4.1.2 => 4.3.5
@MichielDeMey MichielDeMey added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 14, 2021
@mnajdova
Copy link
Member

Thanks for the report @MichielDeMey

Ideally we should move all hooks to the @material-ui/unstyled package, so that you don't need to depend on @material-ui/core.

@michaldudak should we do this? Depending on the @material-ui/core at this moment requires developers to install emotion too.

@mnajdova mnajdova added new feature New feature or request package: base-ui Specific to @mui/base and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 14, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 14, 2021

@mnajdova While I agree with all you said, I would like to raise a quick-win opportunity.

AFAIK the main issue of @MichielDeMey is that tree shaking is disabled in dev mode, usePagination imports core/utils, that imports SvgIcon, that imports system, that import styled, that imports emotion => his codesandbox break. So we force him to install emotion, and he might think that emotion will be bundled in production (it won't, tree shaking kicks in prod mode).

Now, we could force the treeshaking in dev mode and call it a day 😁:

diff --git a/packages/material-ui/src/usePagination/usePagination.js b/packages/material-ui/src/usePagination/usePagination.js
index 4f15278cb1..561747a5e4 100644
--- a/packages/material-ui/src/usePagination/usePagination.js
+++ b/packages/material-ui/src/usePagination/usePagination.js
@@ -1,4 +1,4 @@
-import { useControlled } from '../utils';
+import { unstable_useControlled as useControlled } from '@material-ui/utils';

 export default function usePagination(props = {}) {
   // keep default values in sync with @default tags in Pagination.propTypes

emotion is an optional peer dependency, nothing to install, no warnings.

Then fix the problem at the root (not the surface) with the unstyled package (to remove any fear that the developers might have, for social proof, etc.) in mui/base-ui#10.

@oliviertassinari oliviertassinari added OCD21 component: pagination This is the name of the generic UI component, not the React module! labels Jul 14, 2021
@oliviertassinari oliviertassinari changed the title v5 usePagination requires @emotion [Pagination] v5 usePagination requires @emotion Jul 14, 2021
@MichielDeMey
Copy link
Author

@oliviertassinari I did verify that emotion is not bundled in the final release bundle due to tree shaking. 😉
Just additional download overhead for devs at this point. 😅

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 14, 2021

The same issue with useAutocomplete in v5, we didn't have this problem in v4, cc @siriwatknp. I'm unsubscribing.

@michaldudak
Copy link
Member

@michaldudak should we do this? Depending on the @material-ui/core at this moment requires developers to install emotion too.

Sure, seems like a pretty low hanging fruit. There is a few more components that could simply be moved to unstyled. I'll focus on these before taking on the next "big" one.

@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pagination This is the name of the generic UI component, not the React module! new feature New feature or request package: base-ui Specific to @mui/base ready to take Help wanted. Guidance available. There is a high chance the change will be accepted v5.x migration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants