[Accepted] ADR: Use Box + sx to author components

[siddharthkp]

There are 2 common ways to create components with primer/react:

1. with styled-components
2. with Box + sx

This ADR recommends using the 2nd one exclusively. See rendered markdown →

Next step after ADR is accepted: Update contributor docs.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 845e32a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	65.55 KB (0%)
dist/browser.umd.js	65.9 KB (0%)

[colebemis]

Thanks for writing this, @siddharthkp! 💖

[colebemis]

Can you expand on the issues we've had with exporting types?

[siddharthkp]

sorry for my lazy phrasing here 😅

What I'm trying to convey here is that

1. We've had a couple of bugs like #1959 where we were extracting types from a "styled component"
2. Folks on the memex side have reported that the typew are hard to debug because they have a lot of layers (styled components + styled system)

I feel more confident having more control over what we export + keeping it as flat as possible. Types deserves an ADR of it's own, to be honest.

[lesliecdubs]

Would it be useful to amend the ADR to include some of these additional details for context, either now or in a follow-up PR?

[siddharthkp]

Going to create a new ADR for types (in new PR), there's more to discuss there because we don't have a standard way right now to propose :)

[lesliecdubs]

As discussed among the PRC maintainers group, let's try assigning a decision-maker here so we can move this ADR toward a conclusion.

ADR decision maker: @rezrah
Decision deadline: Friday, 22 April

I'm going to work on more enduring documentation about this process, but in the meantime:

Your role as decision maker is to gather feedback, probe for discussion and disagreement, and then make an informed decision based on the feedback, identified risks, and trade-offs about whether to adopt the ADR or not. Your role is not to decide based on your own personally preferred approach.

Keep in mind that ADR decisions can be reversed or changed in the future if new information is exposed that make the ADR worth revisiting.

You can fulfill your decision maker role by doing the following:

• review the ADR, comments, and feedback closely
• seek additional feedback from any necessary parties, as needed; for example, request a review from any individuals or teams that you know would be unduly affected by this ADR, or who have extensive experience in using either the preferred or non-preferred approach and may have perspective to share about it
• follow up on any remaining concerns, questions, or unexamined risks around accepting the proposed approach
• on or before the deadline, make a decision as to whether the ADR is accepted ✅ , which you should signal by approving this PR, or rejected 🚫 , which you should signal by closing this PR with a comment explaining the reason(s)

[rezrah]

@siddharthkp - can you link to your research, which you mentioned in this comment from the ADR itself for easier discovery? I think it will help to elaborate why working with Primitives might be easier for anyone reading it later.

[siddharthkp]

Done!

[dusave]

Our goals as customers/consumers are:

1. Ease-of-use in terms of authorship (Be it specifying colors, styles, customization, etc)
2. Implementing something that is performant with little to no friction
3. Strongly typed guardrails, so it's easier to implement ("Do I preface this color with colors.? Do I pass a theme here? What does 'space.2' mean?")

I see some pros and cons to this approach:
Pros:

• This allows consumers to more easily define their styles without as much overhead
• It's coupled with the component, so there is less mental friction
• It still provides a solution that is responsive based on props (i.e. dynamic styles)

Cons:

• The biggest drawback I see is that these styles, because they are implemented inside of the component, would be re-computed every single render cycle. While this might be improved with memoization, I still think there's a good deal of perf impact with this approach
• I'm not a huge fan of each component needing to run merge() to get styles to work properly, not to mention the added dependency that each app would need to install
• I'm concerned about the mental friction of implementing everything as a Box and transmuting them into other components by way of as. I always thought of that as an escape hatch more than a common pattern.

I definitely think there are improvements to be made on the implementations of styles, specifically styled-components, but I'm also curious to see what styled-components' plans are now that React 18 is released with all of their improvements and DX hooks for these sorts of libraries.

[mattcosta7]

I don't think the intention is to require a dependency on merge @dusave but just that it's the consumers job to merge local and props sx if necessary?

if Box is a Memo component, could we avoid some overhead by doing a deeper comparison on sx props

memo(function Box(props) {...}, function shouldComponentRender(prevProps, nextProps) {
   compare sx deepy, but maintain as object.is equality otherwise
})

which might avoid some/many performance differences from inline definitions of sx?

[mattcosta7]

I'm concerned about the mental friction of implementing everything as a Box and transmuting them into other components by way of as. I always thought of that as an escape hatch more than a common pattern.

this, i think would be just something to extract to module scope and use from there, except when needing the props. how much non-primer usage actually changes based on props though?

[siddharthkp]

I'm glad we're talking about this. the sx implementation could use a lot of improvements!

The goal of the ADR to standardise using Box + sx for creating new components in primer/react (we are already doing that for a few months), so we can prioritise solving the remaining problems with this approach.

The biggest drawback I see is that these styles, because they are implemented inside of the component, would be re-computed every single render cycle. While this might be improved with memoization, I still think there's a good deal of perf impact with this approach

You're not wrong. We need work performance improvements to make sure the page doesn't slow down over time because of re-renders.

However, injecting styles is the biggest task in the whole process. Because Box still uses styled-components to inject styles in the end, the same styles are not injected twice. The exception to this is prop-based styles which need to be re-evaluated and the additional work required by styled-system on top of styled-components could be a bad fit for use cases like javascript-based animations.

I'm not a huge fan of each component needing to run merge() to get styles to work properly, not to mention the added dependency that each app would need to install

Same. I have a few API ideas on how to simplify the API and abstract the merge away from the component.

For the merge dependency, right now it comes from import {merge} from '@primer/react', but with API changes we should be able to remove it from the public API.

I'm concerned about the mental friction of implementing everything as a Box and transmuting them into other components by way of as. I always thought of that as an escape hatch more than a common pattern.

Can you expand on the mental friction a bit more.

I think you only need to drop down to Box when you're building a new atomic component. <Box as="img"> is equivalent to styled.img.

For most use cases, you should be able to start with a primer/react component, especially as we get more layout components like Stack. However, to give better html semantics, as prop would be the way to go: <Stack as="section"/>

[dusave]

I think I was thinking about the Box using as in the wrong light. What I was thinking is that we get into a state where our components are loaded with boxes being converted to other components using as, which would cause mental friction in trying to read through the code. Rather than looking at the tag name, you would have to look for the as attribute to determine what it actually is.

That said, if this is relatively contained to Primer and not something we do in the consuming packages, that's probably fine.

[mattcosta7]

Is there any consideration for using sx but recommending it be defined in a static scope, with nested classnames, etc being updated instead of the actual sx object?

I think that avoids consumers using styled-components directly, but also avoids the runtime overhead of sx object equality?

[mattcosta7]

I don't think the intention is to require a dependency on merge @dusave but just that it's the consumers job to merge local and props sx if necessary?

if Box is a Memo component, could we avoid some overhead by doing a deeper comparison on sx props

memo(function Box(props) {...}, function shouldComponentRender(prevProps, nextProps) {
   compare sx deepy, but maintain as object.is equality otherwise
})

which might avoid some/many performance differences from inline definitions of sx?

[mattcosta7]

should we avoid styles here, since it's very close to sounding like inline styles vs localSx? or something like that

should we add types here too?

const localSx: BetterStyledSystemObject (is this still the type to use) = {}

[siddharthkp]

componentStyles or baseStyles can be a better variable here. I don't want to prescriptive about the variable name in this ADR here because it depends on the context. (for example, Button has baseStyles + variantStyles + sizeStyles)

BetterSystemStyleObject is the correct type to use, however it could use some improvements as well. I'lld add that to the ADR. Example sandbox

[mattcosta7]

Totally! Just wanted to make sure we don't cause confusion with inline styles here, more than variable naming!

[mattcosta7]

I'm concerned about the mental friction of implementing everything as a Box and transmuting them into other components by way of as. I always thought of that as an escape hatch more than a common pattern.

this, i think would be just something to extract to module scope and use from there, except when needing the props. how much non-primer usage actually changes based on props though?

[siddharthkp]

Is there any consideration for using sx but recommending it be defined in a static scope, with nested classnames, etc being updated instead of the actual sx object?
I think that avoids consumers using styled-components directly, but also avoids the runtime overhead of sx object equality?

Not yet, I only have that in a prototypes outside of primer/react 😅 I can see that happen in the future