React should be a peer dependencies

[riccardo-forina]

https://github.com/kiegroup/kogito-tooling/blob/master/packages/core-api/package.json#L15-L18

One think I noticed working on the AtlasMap POC is that I was getting the Invalid Hook Call Warning error in the console. Ultimately, it was due to @kogito/core-api declaring React as direct dependency, which causes for multiple versions of React being embedded in the bundle. Since AtlasMap is written solely using hooks, this issue come up immediately.

A quick workaround was to instruct WebPack to consider React an external dependency, but it would be better to make React and React DOM peer dependencies in the core-api package.

[tiagobento]

Hi @riccardo-forina!

Thanks for reporting that. I'm glad to see you're using our packages :)

Initially, we thought that we should be the ones in control of React/ReactDOM versions, since some things that we offer as a library/framework is dependent on the React API. You can simply remove your declaration of React and ReactDOM on your modules so that no conflicting versions exist.

However, I also think that we should give users the flexibility of choosing the exact React version to use. Since peerDependencies are not downloaded by default, we would be making it mandatory to declare your version of React/ReactDOM. More than that, it would be possible to choose an incompatible version of React/ReactDOM, resulting in build failures.

Maybe I'm missing something here, but I'm really not sure yet what's I think is the best approach, so if you could provide more information about why having your own declaring of React/ReactDOM is important, I guess it would help us make a decision :)

[riccardo-forina]

React requires a single instance of React DOM for it to properly work. React and React DOM should be a dependency of the final consumer. Taking the template as an example, React and React DOM should be declared as a dependency of the chrome-extension-pack-simple-react and vscode-extension-pack-simple-react packages that are the "final" outputs. simple-react-editor should also declare React as a peer dependency, since that package acts as a library and not a final output.

I think it's a bit easier if we visualize the problem. I cloned the template repo, instructed webpack to generate the stats file to feed to webpack-bundle-analyzer and run it against the simple-react-editor package and the chrome-extension-pack-simple-react. Here is the result:

simple-react-editor

This package gets bundled together with React, because React is a direct dependency of the core-api package.

chrome-extension-pack-simple-react

This package gets bundled together with React too because React is a direct dependency of the core-api package, but since there is also a dependency on simple-react-editor an additional copy of React gets bundled in the final output.

This leads to a bigger bundle size because of the duplicated version of React, and to bugs like the one in the opening comment.

In general the rule of thumb should be that if you are distributing a reusable module, dependencies should be declared as peer dependencies so to avoid these issues. It's then up to the consumer of the library to add the required dependencies to its project. For example, if I want to use, say, react-table I also have to add react as a dependency to my project.

[tiagobento]

Oh yes... Let me begin by apologising, but the thing is that our kogito-tooling-examples repo is a little bit out of date 😅

@caponetto Is currently working on updating the repo so that these issues are fixed.

On our packages in the kogito-tooling repo, we treat React and ReactDOM as externals (using WebPack) and rely on it being transitively downloaded by @kogito-tooling/core-api. This makes only one "instance" of React being package in our bundles, keeping the bundle size to a minimum.

Hope that makes sense, WDYT?

[riccardo-forina]

I mean, your project your rules :D

Let's say that this would be quite an exception in the npm ecosystem and would limit peers to use the provided template and tools (what if I don't want to use webpack? Sure things like rollup allow for declaring external global dependencies, but I don't see why adding this cognitive burden to users).

[tiagobento]

Yeah, I understand. My main concern with letting the user decide their version of React/ReactDOM is that they choose one that is not compatible with what we deliver. I know peerDependencies is there to warn people about that, but it's still error prone.. I guess we can't have it all, right? :)

Thanks again for raising this issue. We'll create a ticket for that and prioritise it accordingly. Feel free to add more to that if you want!

[tiagobento]

Created https://issues.redhat.com/browse/KOGITO-2573. I'll comment here as we have updates.

[tiagobento]

@riccardo-forina I've been thinking about this issue a lot, but I think here's what makes the most sense to me.

When you're creating Editors in React, the package containing them is responsible for providing a React component that's actually an editor, so essentially, this package is a 'library'. Let's say you create a package called my-editor.

Of course, to run this editor, you'll need an 'app'. That's what we call the Envelope, which is essentially a webapp that runs inside an iframe in a containing 'app', which we call the Channel (e.g. Desktop, VS Code, Chrome Extension etc).

To create this Envelope 'app', we provide a library called microeditor-envelope (we'll probably rename that to envelope-lib, so that's how I'm going to refer to that from now on).

This Envelope 'app' is actually assembled with the help of two libraries -- my-editor and envelope-lib. Let's say that your Envelope application is created in your my-envelope-app package.

With all that said, I think that envelope-lib and my-editor should declare React and ReactDOM as peerDependencies, and the Envelope application should declare them as a dependencies. (See an Envelope application index.ts example here: https://github.com/kiegroup/kogito-tooling-examples/blob/master/packages/chrome-extension-pack-simple-react/src/envelope/index.ts

The ideal hierarchy of packages is as follows:

         my-envelope-app <-- (That's where we declare React and ReactDOM as `dependency`)
         /             \
envelope-lib         my-editor
                          \
                        editor-api

As you might've noticed, we don't have a package called editor-api, making my-editor depend on core-api and envelope-lib, which is not ideal.

This way, only 'app' packages would declare a concrete version of React and ReactDOM, making everything else peer-depend on React and ReactDOM. This gives more flexibility for users of our platform. How does that sound to you? :)

---

NOTE: One thing I'd like to clarify that is not related to this whole conversation above is how we mark a dependency as external. It's the bundlers job to do that (e.g. Webpack), and the reason you're seeing React and ReactDOM bundled inside simple-react-editor on kogito-tooling-examples is because the webpack.config.js is wrong. See https://github.com/kiegroup/kogito-tooling-examples/blob/master/packages/simple-react-editors/webpack.config.js#L30.

This line should actually be like this one: https://github.com/kiegroup/kogito-tooling/blob/master/packages/kie-bc-editors/webpack.config.js#L32.

This makes Webpack bundle the package without its dependencies, making it only contain the classes it exports.

[tiagobento]

cc @manstis @caponetto

[riccardo-forina]

Yup I think I’m with you. If I may, check out tsdx as a library boilerplate generator. It uses rollup under the hood that’s more suited to create libraries than webpack and makes working with this stuff easier.

[tiagobento]

@riccardo-forina Thanks for the tip! :)

I'll keep updating this issue until we have the ideal package hierarchy.

[tiagobento]

We removed the core-api module in favor of a more suitable package organization (see #224). React is no longer a hard-dependency for creatin Editors. Thank you @riccardo-forina for pointing that out! Expect more news around our tooling for creating your own Editors soon. Closing the issue.