-
Notifications
You must be signed in to change notification settings - Fork 244
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
chore(components): use npm overrides to force transitive leafygreen dependencies to the same version of shared packages #7223
Conversation
…ependencies to the same version of shared packages
815378c to
95a8bd5
Compare
…check to include @lg-chat namespace in it
…add the missing export
2a768c3 to
58b2b3f
Compare
|
|
||
| describe('with existing chat instance', function () { | ||
| // TODO: some internal logic in lg-chat breaks all these tests, re-enable the tests | ||
| describe.skip('with existing chat instance', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gagik @lerouxb just wanted to highlight: I can either revert switching assistant to new UI or keep this skip in this PR, whatever you prefer. If we keep the skip, we probably want to follow-up with fixing those right away. At a glance it's just jsdom missing some methods, so shouldn't be too hard, I'm just trying to keep this PR small
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on a follow-up PR so we can just skip this here and I'll rebase and re-add those
gagik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable, thanks for looking into this!
| </WithErrorBoundary> | ||
| ), | ||
| title: ( | ||
| <WithErrorBoundary name={name} type="header"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why the removed error boundary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's this issue caused by the fact that @leafygreen-ui/lib@15 seems like the one that seems to be working for all our dependendecies more or less, but it does cause something weird in leafygreen tabs component specifically: the title that we pass there wrapped in a boundary starts to fail with this error (I'm guessing they are doing something with this value passed that triggers the constructor accidentally). Before tabs were installing their own version, but doing this very targeted override on top of other overrides is a pain in the butt, so I'm just getting around the issue here by removing the boundary from the rendering tree.
I'll leave a todo
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-chatinto compass in this POC) and so one option that we seem to have instead of vendoring 16 different packages that@lg-chatconsists 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