Skip to content

chore(components): use npm overrides to force transitive leafygreen dependencies to the same version of shared packages #7223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gribnoysup
Copy link
Collaborator

@gribnoysup gribnoysup commented Aug 22, 2025

We are currently in a pretty nasty state where we can't easily add new leafygreen dependencies because our local versions are not up to date. Adding new deps in most cases forces us to update other leafygreen dependencies, which causes issues across the whole codebase that are not easy to fix.

This is especially a pain point right now for multiple projects that depend on new leafygreen components that we don't have any way of easily rebuilding ourselves. We already had to vendor in the Drawer component, and the leafygreen chat is even more of an issue because it's basically a whole monorepo of its own that we will have to include.

Lucky for us though, it seems like while there are multiple major version updates in leafygreen, not all of them are meaningfully breaking for components we are trying to add dependencies on, at least at a glance (this is something that @gagik managed to prove by doing an initial pass of vendoring @lg-chat into compass in this POC) and so one option that we seem to have instead of vendoring 16 different packages that @lg-chat consists of, is using npm overrides feature to just force all shared leafygreen dependencies to the same version, this is what this patch is doing.

To be super clear, this is a massive hack: npm is fiddly and sometimes ignores overrides altogether or overrides them back to original values outside of your control on unrelated installs, sometimes behavior is even slightly different between minor npm versions, overrides don't apply to peer dependencies so we still have to force npm install when adding new deps, and we also have no guarantees that this doesn't actually break any of the leafygreen components. At a glance nothing broke, but any next update might not go through at all. Hopefully our tests are good enough to catch this.

At the same time this is still somewhat cleaner than pulling half of leafygreen monorepo as-is in our code. This is hopefully a short lived solution that we will not need to keep in the repo for long and can remove as soon as leafygreen update is finished, but this should allow us to move forward for now in the cleanest way possible.

First commit only makes sure that all existings dependencies are aligned and adds explicit dependencies to two new leafygreen packages that we are currently missing that lg-chat depends on. In the second commit I'll add all lg-chat dependencies and re-export through compass-components. Wanted to open PR already to have some initial CI runs going

…ependencies to the same version of shared packages
@gribnoysup gribnoysup force-pushed the overrides-for-shared-leafygreen-dependencies branch from 815378c to 95a8bd5 Compare August 22, 2025 15:11
@@ -102,6 +102,17 @@ const sharedResolveOptions = (
'index.js'
),

// TODO: document what's going on here
'@leafygreen-ui/button/constants': path.resolve(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a dependency of lg-chat for a single (!) re-exported value that is used in one (!) place that doesn't exist in our version of @leafygreen-ui/button. While we can override it for webpack, this still causes issues for unit tests because those are not bundled (and we wouldn't want to start bundling them). This is probably something we should ask leafygreen team to adjust for us and hardcode it in place where it's used because there is no real way for us to deal with this otherwise (like maybe we can also start using patch-package, but there should be some limit to the amount of hacks we're introducing)

process.env.DISABLE_DEVSERVER_OVERLAY === 'true'
? false
: { warnings: false, errors: true, runtimeErrors: true },
overlay: {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: revert this back, did it by accident

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants