Revert "fix: use lerna to share code instead of copying resources (#214)

[jenniferarnesen]

This reverts commit eecfea0.

The code-sharing solution does not work when the plugin is pulled in as a dependency in other apps (e.g., dashboards), since imports like:

import { apiFetchVisualization } from 'data-visualizer-app/src/api/visualization';

don't exist in dashboards. This code will have to remain duplicated until we are ready with a published, shared library.

[varl]

How are you pulling in the plugin in dashboards?

You will need to create a plugin bundle file which contains all of the dependencies in the bundle. This requires building the plugin with e.g. Webpack.

[varl]

Currently the plugin build is just a transpiled build: https://github.com/d2-ci/data-visualizer-plugin

[amcgee]

I've been trying to work with the existing app/plugin split in DV and I don't find it usable. I really don't think they should exist as separate packages, they should just be separate entrypoints to the same package (i.e. webpack bundles as @varl mentioned). This isn't natively supported by CRA, but it could be layered on top of CRA. It will also be built into the next-gen app build infrastructure.

[jenniferarnesen]

You will need to create a plugin bundle file which contains all of the dependencies in the bundle. This requires building the plugin with e.g. Webpack.

I thought in general we did not want to create bundles, so that we can do treeshaking? If I do this, then I add more customization to the webpack file, which is what we want to move away from.

[amcgee]

I thought in general we did not want to create bundles, so that we can do treeshaking?

I think that's a good goal eventually, but I don't think it's the right optimization here for two reasons:

1. If any dependency in the tree isn't published on NPM, it can't be accessed by the consumer. So if Dashboards App is the consumer which does the bundling and tree shaking, then both plugin and app need to be published to npm (or they need to live together and have separate entrypoints, whether that's pre-bundled or not)
2. Importantly, we're already duplicating all of the shared code between the plugin and the app, so tree shaking between i.e. DV and Dashboards doesn't make a whole ton of sense to me. The solution I'm working towards is using SplitChunksPlugin to pull out the common deps from plugin and app into a separate, cacheable chunk which can be referenced from both plugin and app bundles. That is open to duplication between a host app and a plugin, but it makes plugin for any given app self-contained and doesn't require all app plugins to be known and host-app compile time.

[jenniferarnesen]

I've been trying to work with the existing app/plugin split in DV and I don't find it usable.

Can you be more specific about what you dont find "usable"? Yes, I know it should be tagged so the apps can be assured to get the desired functionality. What else?

I really don't think they should exist as separate packages, they should just be separate entrypoints to the same package (i.e. webpack bundles as @varl mentioned). This isn't natively supported by CRA, but it could be layered on top of CRA. It will also be built into the next-gen app build infrastructure.

This requires more discussion, and doesn't really help me out right now. The issue I'm trying to fix right now is for the dashboard to build with a tagged version of the plugin so we get rid of the plugin issues that people are experiencing.

So yes, this is obviously a short term solution.

[amcgee]

Can you be more specific about what you dont find "usable"? Yes, I know it should be tagged so the apps can be assured to get the desired functionality. What else?

It doesn't link into dashboards because the app code isn't transpiled. Bundling the plugin with webpack would solve this, so that it pulls in the dependent code from the app when it's built. I can take a stab at fixing that if you'd like.

I believe that's an alternative solution to backing out the switch to lerna and going back to using github for dependency resolution.

[jenniferarnesen]

Can you be more specific about what you dont find "usable"? Yes, I know it should be tagged so the apps can be assured to get the desired functionality. What else?

It doesn't link into dashboards because the app code isn't transpiled. Bundling the plugin with webpack would solve this, so that it pulls in the dependent code from the app when it's built. I can take a stab at fixing that if you'd like.

Go ahead :)

[varl]

I really don't think they should exist as separate packages, they should just be separate entrypoints to the same package (i.e. webpack bundles as @varl mentioned).

App to app dependencies is a messy system architecture, and I do not think it's a good idea when considering the larger picture of our ecosystem. App to app dependencies is one point I am unwilling to budge on until we have exhausted all other options.

Having them in separate packages allows us to be explicit about which app has a plugin and which does not, and other apps can depend on the plugin. It's not a perfect solution but I vastly prefer it over app to app dependencies.

I thought in general we did not want to create bundles, so that we can do treeshaking?

When creating the plugin.bundle.js Webpack will tree-shake what it can from that bundle.

IIRC that plugin exposes a single component which presumably uses everything left in the bundle, so there shouldn't be anything left to shake out?

[varl]

I believe that's an alternative solution to backing out the switch to lerna and going back to using github for dependency resolution.

Can we try the bundle route first?

[jenniferarnesen]

App to app dependencies is a messy system architecture, and I do not think it's a good idea when considering the larger picture of our ecosystem. App to app dependencies is one point I am unwilling to budge on until we have exhausted all other options.

Having them in separate packages allows us to be explicit about which app has a plugin and which does not, and other apps can depend on the plugin. It's not a perfect solution but I vastly prefer it over app to app dependencies.

I have another PR that I'm awaiting feedback on, where I am trying to figure out how to share other code. I agree that the "app" should not contain the shared code. #206

When creating the plugin.bundle.js Webpack will tree-shake what it can from that bundle.

IIRC that plugin exposes a single component which presumably uses everything left in the bundle, so there shouldn't be anything left to shake out?

It was more about what the dashboard ends up bundling. In the case of this plugin, there probably isn't much common code between dashboard and what the plugin uses, it was more of a principle thing.

[amcgee]

IIRC that plugin exposes a single component which presumably uses everything left in the bundle, so there shouldn't be anything left to shake out?

Things like Mui aren't de-duplicated when bundled at the plugin level instead of at the host app (Dashboards) level (Dependencies could theoretically exist in the host app and also every plugin). So yeah, we're good for tree-shaking but not for de-duplication.

App to app dependencies is a messy system architecture, and I do not think it's a good idea when considering the larger picture of our ecosystem. App to app dependencies is one point I am unwilling to budge on until we have exhausted all other options.

Having them in separate packages allows us to be explicit about which app has a plugin and which does not, and other apps can depend on the plugin. It's not a perfect solution but I vastly prefer it over app to app dependencies.

I agree we should avoid arbitrary app-to-app dependencies (which is exactly what we have here, just with a "plugin" shim between the two apps). However, having two different "artifacts" of an app build (one app, one plugin) seems quite a bit safer.

[amcgee]

Can we try the bundle route first?

Yes, let's.

[varl]

It was more about what the dashboard ends up bundling. In the case of this plugin, there probably isn't much common code between dashboard and what the plugin uses, it was more of a principle thing.

Ah, as a principle, yes. Bundling here is to strike a pragmatic balance.

So yeah, we're good for tree-shaking but not for de-duplication.

You're right, that's an important distinction.

[varl]

I agree we should avoid arbitrary app-to-app dependencies (which is exactly what we have here, just with a "plugin" shim between the two apps).

Agreed, though that "shim" layer at least keeps the system level architecture some-what sane and I can maintain a graph of how the apps interact. If we start to introduce inter-app dependencies before we've even gotten rid of the single one we have (core-resource-app) my brain is going to melt.

However, having two different "artifacts" of an app build (one app, one plugin) seems quite a bit safer.

Essentially that is what was the plan with using Lerna for these app+plugin packages. Each build would produce two artifacts: one app artifact and one plugin artifact.

The app would be used for DHIS2 and the plugin would be published to NPM so other apps can use it. There are different technical was to go about this, and there are some drawbacks to each, but the principle of creating two artifacts from the same source is the same.

[jenniferarnesen]

I've merged this. Here is the commit message:

Revert "fix: use lerna to share code instead of copying resources (#214)" (#216)

This reverts commit eecfea0dd13e7620e0dd5c1f104e8aba0a3b5505.

This reintroduces code duplication that had been removed. The implementation causes the plugin to break in dashboards app. Reverting this now and will re-introduce a better solution with webpack bundling soon.

The reason this is being rushed in now is because we are migrating the data-visualizer-app to a mono-repo.

[dhis2-bot]

🎉 This PR is included in version 33.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[dhis2-bot]

🎉 This PR is included in version 32.1.0 🎉

The release is available on:

• npm package (@release-32.x dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[dhis2-bot]

🎉 This PR is included in version 33.1.13 🎉

The release is available on:

• npm package (@release-33.x dist-tag)
• GitHub release

Your semantic-release bot 📦🚀