Stanko / react-animate-height

Lightweight React component for animating height using CSS transitions. Slide up/down the element, and animate it to any specific height.
https://muffinman.io/react-animate-height
MIT License
758 stars 53 forks source link

🪲 Bug: Doesn't work with Webpack Module Federation #140

Closed tgelu closed 1 year ago

tgelu commented 1 year ago

The two package.json files from the build output esm/package.json and cjs/package.json which each contain one line confuse webpack module federation and consequently do not load properly in a federated micro frontend app.

Code example I cannot show a code example but I can explain how the error manifests itself. Webpack's Module Federation does many things, but at some point it tries to understand the installed version of each dependency. When it reaches react-animate-height it looks for the package.json and reads the version from there. But because react-animate-height has more nested package.json files (which isn't a standard) it finds the deepest one which only contain the type property and isn't a valid package.json file. This confuses the build system because it sees a dependency which is installed but with no version or name.

The error from webpack looks like this:

WARNING in shared module react-animate-height -> /node_modules/react-animate-height/dist/esm/index.js
No version specified and unable to automatically determine one. No version in description file (usually package.json). Add version to description file, or manually specify version in shared config.

Expected behavior react-animate-height should work in a federated app. The build output should not contain any other package.json files than the root one. (as an aside: I am pretty sure the import and require entries in the root package.json are enough for build systems to know where to locate the entry points, this is pretty standard. I know of no reason to add those extra package.json files)

Possible Solution In the root package.json:

 - "build:esm": "tsc -p tsconfig.json && echo '{ \"type\": \"module\" }' > dist/esm/package.json",
 - "build:cjs": "tsc -p tsconfig-cjs.json && echo '{ \"type\": \"commonjs\" }' > dist/cjs/package.json",
 + "build:esm": "tsc -p tsconfig.json",
 + "build:cjs": "tsc -p tsconfig-cjs.json",

Steps to Reproduce (for bugs)

  1. Checkout any example of two apps loading each other in a micro frontend with webpack Module Federation (like this one)
  2. Install reeact-animate-height in both apps, the host and the remote.
  3. Try to run the apps and see them fail.

Screenshots N/A

Your Environment N/A

Additional context We're building a micro frontend application with webpack and can't use this package.

Stanko commented 1 year ago

Hi @tgelu, Thank you for the detailed description. I haven't used module federation so far, but I'll try to setup a small app and look into it this week.

The reason for two package.json files is that bundlers do get confused in some cases (this post describes issues I encountered).

But if I read it correctly, the error is about missing version. Do you think that adding a version to generated package.json files would be enough?

Cheers!

tgelu commented 1 year ago

I am not familiar with the issues described in detail in that article. I have not had any major issue with using the exports field with import and require. Something like the below:

    "exports": {
        ".": {
            "import": "./dist/mjs/index.js",
            "require": "./dist/cjs/index.js"
        }
    }

But I may lack some knowledge of edge cases or other things, so I'm not going to comment on that.

Do you think that adding a version to generated package.json files would be enough?

From my own tests, seems like it would be enough, yes.

PS: it still seems a little bit like those extra package.json files try to "trick" some build systems in some ways, so IMHO it is still not the prudent choice :D. But this is besides the point.

Stanko commented 1 year ago

For now, I patched it in version 3.1.2 by adding version to both package.json files:

But I will look into solving it properly.

EDIT: I would be grateful if you can check if this works around the problem.

tgelu commented 1 year ago

Seems to be working now :) - thanks a lot!

Feel free to close the issue.

tgelu commented 1 year ago

UPDATE:

I did not test properly. The version warning is gone but now I get a new warning:

[webpack-dev-server] WARNING in shared module react No required version specified and unable to automatically determine one. Unable to find required version for "react" in description file (node_modules/react-animate-height/dist/esm/package.json). It need to be in dependencies, devDependencies or peerDependencies.

So I am guessing webpack is still confused about this package's package.json file :(

Stanko commented 1 year ago

Heh, It was too easy to be true.

I'll try to publish a beta version today with the solution you proposed initially. Would you be so kind to test it, so I can publish the fix afterwards?

Stanko commented 1 year ago

@tgelu I published a beta version which removes additional package json files: https://cdn.jsdelivr.net/npm/react-animate-height@3.1.3-beta.1/package.json

From what I can see, it works with both CJS and ESM modules.

Please test it yourself and I'll publish a new version

npm i react-animate-height@beta
tgelu commented 1 year ago

Getting no warnings on the @beta release 👍

Stanko commented 1 year ago

Thanks for checking.

I've just published v3.2.0, I think that is it.

tgelu commented 1 year ago

Thank you!