react-hook-form-mui icon indicating copy to clipboard operation
react-hook-form-mui copied to clipboard

Use split bundle to reduce dependency requirements

Open paales opened this issue 3 years ago • 9 comments

Duplicates

  • [X] I have searched the existing issues

Latest version

  • [X] I have tested the 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?

paales avatar Aug 10 '22 09:08 paales

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

dohomi avatar Aug 11 '22 23:08 dohomi

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?

paales avatar Aug 26 '22 10:08 paales

@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.

dohomi avatar Aug 29 '22 04:08 dohomi

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?

paales avatar Aug 29 '22 08:08 paales

@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)

dohomi avatar Aug 29 '22 08:08 dohomi

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

paales avatar Aug 29 '22 09:08 paales

@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 avatar Sep 07 '22 19:09 paales

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

paales avatar Sep 07 '22 19:09 paales

@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 avatar Sep 09 '22 06:09 dohomi

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 avatar Oct 28 '22 01:10 dohomi

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

dohomi avatar Nov 28 '22 11:11 dohomi