dohomi / react-hook-form-mui

Material-UI form components ready to use with react-hook-form
https://react-hook-form-material-ui.vercel.app
MIT License
530 stars 108 forks source link

Use split bundle to reduce dependency requirements #86

Closed paales closed 1 year ago

paales commented 2 years ago

Duplicates

Latest version

Summary 💡

Currently the build version is using a single compiled file which makes the requirement on @mui/icons-material and @mui/x-date-pickers a requirement. The package.json indicates that they are meant to be optional: https://github.com/dohomi/react-hook-form-mui/blob/master/package.json#L44-L51

We don't use the optional libraries but it seems they are required when using the package.

Examples 🌈

Maybe move to esm packages so we can actually import the components directly?

import TextInputElement from 'react-hook-form-mui/TextInputElement' or something like that?

dohomi commented 2 years ago

This package is using tsup under the hood, I thought this should take care of the code split?

paales commented 2 years ago

I don't think we need to bundle the packages as that creates a single large bundle that includes everything and causes problems with treeshaking.

Was looking at the following guides:

https://cmdcolin.github.io/posts/2022-05-27-youmaynotneedabundler https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#package-json-exports-imports-and-self-referencing

And I seem to be able to achieve this with these two commands:

yarn tsc --noEmit false --outDir dist/esm
yarn tsc --noEmit false --outDir dist --target ES5 --module CommonJS

Some modifications seem to be required to the package.json according to the typescript blogpost

{
"exports": {
    ".": {
      "import": {
        "types": "./dist/esm/index.d.ts",
        "default": "./dist/esm/index.js"
      },
      "require": {
        "types": "./dist/index.d.ts",
        "default": "./dist/index.js"
      }
    }
  },
  "types": "dist/index.d.ts",
  "main": "dist/index.js",
}
Schermafbeelding 2022-08-26 om 11 51 56

I can try and create a PR, do we have the ability to create a preview/beta/canary release so we can test it out first?

dohomi commented 2 years ago

@paales I restructured the repo to run as a mono repo. With this I had a better overview in what is actually been bundled and how the footprint of for example NextJS with a minimal form looks like. The analyzer shows that there is nothing to complain about, only the components are bundled which are actually in use.

I think the esbuild too only bundles what is actually used.

paales commented 2 years ago

Hi @dohomi!

Could you help me figure things out? We're trying to use the package but we're not using @mui/x-date-pickers, but at the moment everything is bundled in a single file which causes issues.

If you go to https://www.npmjs.com/package/react-hook-form-mui and click on Explore you can see that it is bundled.

When using it in our nextjs installation we get the issue that it is missing the package and erroring. Maybe there is a different solution?

dohomi commented 2 years ago

@paales what framework on top of React are you using? CRA or NextJS? Did you have a look at the https://github.com/dohomi/react-hook-form-mui/tree/master/apps/nextjs where I dont use @mui/x-date-pickers and it is also not part of the bundle. This is a real world scenario and it should work for you the same (if you use NextJS)

paales commented 2 years ago

Ok, I'll have to get back to you :)

paales commented 2 years ago

@dohomi Found some time to take a deeper look. I'm getting the following error currently:

error - ../../node_modules/react-hook-form-mui/dist/esm/index.js:1:1492
Module not found: Can't resolve '@mui/x-date-pickers/DatePicker'

Import trace for requested module:
../../packages/ecommerce-ui/components/FormComponents/index.ts
../../packages/ecommerce-ui/components/index.ts
../../packages/ecommerce-ui/index.ts
../../packages/magento-cart/components/CartFab/CartFab.tsx
../../packages/magento-cart/components/index.ts
../../packages/magento-cart/index.ts
./lib/graphql/GraphQLProvider.tsx
./pages/_app.tsx

https://nextjs.org/docs/messages/module-not-found
paales commented 2 years ago

Also, when copying the files I get the following bundlesize increase (expected):

https://github.com/graphcommerce-org/graphcommerce/pull/1579#issuecomment-1239788897

When using the library as is (with the missing dependencies installed) I'm getting the following bundlesize:

https://github.com/graphcommerce-org/graphcommerce/pull/1579#issuecomment-1239801390

dohomi commented 2 years ago

@paales I am currently busy with work, can you provide a PR to fix the current behaviour? Otherwise Ill look into it as soon I get some time. I personally never used this library without the DatePicker thats why I probably never encountered the issue.

dohomi commented 1 year ago

Also, when copying the files I get the following bundlesize increase (expected):

graphcommerce-org/graphcommerce#1579 (comment)

When using the library as is (with the missing dependencies installed) I'm getting the following bundlesize:

graphcommerce-org/graphcommerce#1579 (comment)

@paales the increased size is totally expected, don't you think? Also the size seems fair as you are depending now on react-hook-form with the <TextFieldElement> component. It does not seem to include any size which your are not currently using.

dohomi commented 1 year ago

I'm closing the issue seems to be working as expected.

kevinpastor commented 1 year ago

I'm having the same issue. I have a repo where I'm not using @mui/x-date-pickers but I need to add it as a dependency if I want to use this library.

I'm using Next.js and pnpm and I'm getting the following error:

./node_modules/.pnpm/react-hook-form-mui@5.12.3_oubwgxn3piashzxhroobwyl2zy/node_modules/react-hook-form-mui/dist/esm/index.js:1:1516
Module not found: Can't resolve '@mui/x-date-pickers/DatePicker'

Import trace for requested module:
./src/pages/add.tsx

https://nextjs.org/docs/messages/module-not-found
dohomi commented 1 year ago

@kevinpastor you might need to add the dependency inside of your package as devDependency but your final build wont include if not used