Monorepo ADR #1191
Monorepo ADR #1191
Conversation
b-kelly
force-pushed
the
bkelly/monorepo-adr
branch
from
a8f49ca to
85c9689
Compare
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
giamir
left a comment
giamir
left a comment
There was a problem hiding this comment.
Thanks @b-kelly for getting this started. ❤️
I left few comments here and there but overall I like where this is going.
We should think about how to prioritize the work and get started. This is purely technical work so it might fit into that 30% (ballpark) of tech tasks we should pick up every iteration.
Part of this work will also become a blocker when we want to start to build more complex UI components. I really like you brought this up already.
I suggest to finalize the ADR, merge it and then start breaking down what we need to do in bite-size tech tasks. Happy to support there and also excited to pick some of those tasks up. 🙂
|
|
||
| Our current codebase could be split like so: | ||
| - Atomic classes (no dependencies) | ||
| - CSS Components (depends on Atomic components) |
There was a problem hiding this comment.
I would prefer to keep CSS components and JS components in the same package (in the spirit of a true component based architecture - see also folder-by-feature vs folder-by-type). I would even go a step further and create folders for individual components where we keep style (less), behavior (stimulus - when applicable) and component test files (tbd - perhaps wtr + testing-library) together.
In an ideal scenario even the "docs" would be in the same component folder but I understand that it won't be an easy thing to achieve given our current setup. I would be ok to settle for docs in a separate package (revaluate perhaps at a later stage).
There was a problem hiding this comment.
The reason I had suggested we separate the two is two-fold:
- If you only need the Stacks classes and not the JS components, you don't have to pull them in
- We're looking to modernize our JS component story, which means potentially having more than one supported component library (e.g. Stimulus + Web Components / React / Svelte / whatever), at least for a short while
That being said, I'm open to discussion on this point. I'm very much interested in hanging with the team, hashing out the pros / cons and coming to a hard decision. I'll schedule something for us for before the end of the calendar year.
There was a problem hiding this comment.
Point 1 could be achieved even if we ship a single npm "stacks-components" package.
We will still have a way for consumers to opt in for css only if they want to I suppose.
The reason why I am suggesting for the component styles to go hand in hand with Stimulus is because I don't see us reusing any of that source code (Stimulus and Less) when we will build web components (or anything other component library).
My thinking as of now is that new components will depend exclusively on the atomic classes package and work in any context without depending on global styles being applied to a page or not.
I believe styles will have to be rewritten but I might be miscalculating things. Discovery will certainly help.
In case we will reuse the same component styles we have today in Stacks with the new components then I would understand why we would keep components styles in a separate package.
There was a problem hiding this comment.
It's just my ¢2, but I agree with Giamir that a single package for both CSS components and JS components makes the most sense.
On top of Giamir's points, it's plausible that we could convert a CSS component to a JS component at some point. Correct me if I'm wrong, but I imagine we'd need to deprecate the component in the CSS package and port it to the JS component package. That seems like it would be messy and a single Stacks components package makes more sense to me intuitively.
There was a problem hiding this comment.
I think this is the only pending topic before we could merge this ADR too. 🙂
There was a problem hiding this comment.
This is actually something I feel strongly about and would be better suited to discuss over a hangout. In the meanwhile, I'm going to go ahead and merge as-is, since that line is just a suggestion and implementation can differ from what I have here.
We can always come back and update this later. The PR for that will serve as its own little mini ADR, since it'll contain the reasoning for the change.
There was a problem hiding this comment.
I have mixed feelings about this PR being merged in without reaching an agreement on this point first. This is primarily because what we merge we consider automatically as approved (we don't have a status flag).
Can I suggest to add a comment/line directly in the ADR content to remind ourselves that further discussion is required about packages organisation.
Don't get me wrong, I like to move things forward fast but I also think it's ok to slow down a bit when we cannot agree on something as a team. @b-kelly Can I leave this small adjustment with you together with scheduling a meeting for us to talk further about this topic when the time is right? Thanks and sorry if this request sounds a bit pedantic. 🙂
There was a problem hiding this comment.
@giamir I'll make an adjustment. I see this section as a "for example" section, not as something I was proposing in this PR. The wording could be alludes to that. Would it make more sense if I reworded it like:
For example, our current codebase could be split like so
There was a problem hiding this comment.
Sounds good to me. Thanks.
I guess the more important action item is to come together as a team, take a decision and retrospectively justify our reasoning in this ADR too (we are not in a rush though). 🙂
| - Atomic classes (no dependencies) | ||
| - CSS Components (depends on Atomic components) | ||
| - JS Components (depends on Stimulus, Popper.js, CSS + Atomic components) | ||
| - Documentation site (depends on JS components, 11ty + plugins) |
There was a problem hiding this comment.
if we keep js and css components in the same package then this will depends on a "stacks-components" package and transitively on the atomic classes package (perhaps "stacks-base")
| When the codebase expands in the future, we'll be able to add the following without worrying about consumers needing to import dependencies they don't need: | ||
| - Front-end component library or libraries (e.g. React, Web Components) | ||
| - Large, complex components (e.g. autocomplete) | ||
| - Extra build chain items, such as a [Style Dictionary](https://amzn.github.io/style-dictionary/) |
There was a problem hiding this comment.
Nice that you mention a design tokens tool here - it might be something worth exploring in the future (especially if we would have to start supporting non-web platforms too). 🙂
There was a problem hiding this comment.
I did some exploration on this during my personal "Learn, Share, Grow" day. Technically, we already support a "non-web" (non-CSS?) platform - Figma 😉
| - Large, complex components (e.g. autocomplete) | ||
| - Extra build chain items, such as a [Style Dictionary](https://amzn.github.io/style-dictionary/) | ||
|
|
||
| We'd also be able to fold the [Stacks-Icons](https://github.com/StackExchange/Stacks-Icons) and [Stacks-Utils](https://github.com/StackExchange/Stacks-Utils) repos into this main monorepo as well. |
|
|
||
| ## Additional Info | ||
|
|
||
| On the technical front, we could manage the monorepo with [npm workspaces](https://docs.npmjs.com/cli/v9/using-npm/workspaces?v=true) or another dedicated tool such as [Rush](https://rushjs.io/pages/intro/welcome/), [Lerna](https://lerna.js.org/) or [Bit](https://bit.dev/). |
There was a problem hiding this comment.
I have had mixed experiences with npm workspaces in my previous team (e.g. poor support of selective deps resolutions, upgrade deps pain, etc...) but in the end it did its job and it's nice you don't need to install any extra tool. Workspaces in yarn is way more mature though (or at least it was when I evaluated both tools half a year ago or so - maybe npm caught up a bit in the meantime).
There was a problem hiding this comment.
Definitely down to do some discovery on this. Ultimately, I don't really care what tool we use so long as it stays out of the devs' way.
|
|
||
| On the technical front, we could manage the monorepo with [npm workspaces](https://docs.npmjs.com/cli/v9/using-npm/workspaces?v=true) or another dedicated tool such as [Rush](https://rushjs.io/pages/intro/welcome/), [Lerna](https://lerna.js.org/) or [Bit](https://bit.dev/). | ||
|
|
||
| In regards to changelog / release notes generation, we'd continue using Conventional Commits, but moving to use [release-please](https://github.com/googleapis/release-please) which [does support monorepos](https://github.com/googleapis/release-please/blob/main/docs/manifest-releaser.md). |
There was a problem hiding this comment.
I don't have hands on experience with release-please but I am excited to try that out. I believe we should automatize our release process as much as possible (including publishing packages to npm).
I have used conventional commits and I really like it. We might be even able to enforce those conventions with commitlint and nudge people in the right direction with comittizen (in my previous project most teammates they defaulted to run npx cz instead of using the git cli to create commits). 🙂
There was a problem hiding this comment.
I've played with release-please only a little, but I've been using commit linting (plus a GH action to enforce) over in Stacks-Editor for a while now to great success. It's something that we've unofficially adopted across all of the Stacks repos recently, but haven't yet had the opportunity to add formal enforcement mechanisms.
There was a problem hiding this comment.
I've used Commitizen in the past and appreciated it (but I'm open). If we used Commitizen, I'd suggest configuring it on the side of warn to start, because it can get too annoying to have it yell at you for trying to commit with a message like cleanup when you're gonna squash it anyways. I want to make sure our tooling resides in the Goldilocks zone of annoyingness.
dancormier
left a comment
dancormier
left a comment
There was a problem hiding this comment.
Overall, I'm pretty 👍 on this ADR
|
|
||
| Our current codebase could be split like so: | ||
| - Atomic classes (no dependencies) | ||
| - CSS Components (depends on Atomic components) |
There was a problem hiding this comment.
It's just my ¢2, but I agree with Giamir that a single package for both CSS components and JS components makes the most sense.
On top of Giamir's points, it's plausible that we could convert a CSS component to a JS component at some point. Correct me if I'm wrong, but I imagine we'd need to deprecate the component in the CSS package and port it to the JS component package. That seems like it would be messy and a single Stacks components package makes more sense to me intuitively.
|
|
||
| On the technical front, we could manage the monorepo with [npm workspaces](https://docs.npmjs.com/cli/v9/using-npm/workspaces?v=true) or another dedicated tool such as [Rush](https://rushjs.io/pages/intro/welcome/), [Lerna](https://lerna.js.org/) or [Bit](https://bit.dev/). | ||
|
|
||
| In regards to changelog / release notes generation, we'd continue using Conventional Commits, but moving to use [release-please](https://github.com/googleapis/release-please) which [does support monorepos](https://github.com/googleapis/release-please/blob/main/docs/manifest-releaser.md). |
There was a problem hiding this comment.
I've used Commitizen in the past and appreciated it (but I'm open). If we used Commitizen, I'd suggest configuring it on the side of warn to start, because it can get too annoying to have it yell at you for trying to commit with a message like cleanup when you're gonna squash it anyways. I want to make sure our tooling resides in the Goldilocks zone of annoyingness.
|
@b-kelly heads up: before merging remember to change the sequential number to |
Adds an ADR detailing the "why" and partial "how" for migrating Stacks to a monorepo with multiple packages.