Propose a new file structure (ADR) #1678

jfuchs · 2021-12-01T19:33:38Z

This PR adds a proposed ADR establishing a file convention.

There are two reasons I'm doing this:

I think the result would be good (more to come on that in the ADR)
I'm interested in seeing if we can keep ADRs fast and light for smaller questions

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2021-12-01T19:33:41Z

⚠️ No Changeset found

Latest commit: f252635

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 · 2021-12-01T19:37:51Z

size-limit report 📦

Path	Size
dist/browser.esm.js	57.5 KB (0%)
dist/browser.umd.js	57.8 KB (0%)

contributor-docs/adrs/adr-XXX-file-structure.md

colebemis · 2021-12-01T21:19:01Z

Having a consistent file structure would also lay the groundwork for us to create a npm run new:component script to scaffold a new component 😄

contributor-docs/adrs/adr-XXX-file-structure.md

rezrah · 2021-12-02T09:42:32Z

contributor-docs/adrs/adr-XXX-file-structure.md

+Questions I would like to settle in this ADR:
+
+- On subcomponents:
+  - Do subcomponents filenames need to be namespaced with the parent component? (e.g., `BreadCrumbsItem.tsx` vs `Item.tsx`)


➕ : Makes searching for related files easier in the IDE

Have to say first - super happy to go with what the majority of the team prefers. very loosely help opinions.

Aw, I was going to vote ➖ on this because it is scoped inside the component folder: src/Breadcrumb/Item.tsx.

For example, in VS Code, i would search for "action item" to look for ActionList/Item

FWIW, I prefer the longer more descriptive name.

rezrah · 2021-12-02T09:46:19Z

contributor-docs/adrs/adr-XXX-file-structure.md

+
+- On subcomponents:
+  - Do subcomponents filenames need to be namespaced with the parent component? (e.g., `BreadCrumbsItem.tsx` vs `Item.tsx`)
+  - Should the `index.ts` in each component directory export every subcomponent under its own name, or should we prefer `Object.assign`ing subcomponents to the parent component?


Could it depend on whether the subcomponent is private to its parent and not intended for composition? I.e. With a compound component, I'd expect it the parent component to be the only one exported from the folder. Might be misunderstanding this point though.

I think I've seen two different styles:

BreadCrumbsItem, which isn't exported on their own and is only accessed as BreadCrumbs.Item

ButtonDanger, which is available as its own export.
it is reasonable to me that those are different, as BreadCrumbsItem only makes sense as a child of BreadCrumbs.... and maybe we could name that as a rule? Alternately, we could consolidate to one style, right?

rezrah · 2021-12-02T09:48:06Z

contributor-docs/adrs/adr-XXX-file-structure.md

+  - Do subcomponents filenames need to be namespaced with the parent component? (e.g., `BreadCrumbsItem.tsx` vs `Item.tsx`)
+  - Should the `index.ts` in each component directory export every subcomponent under its own name, or should we prefer `Object.assign`ing subcomponents to the parent component?
+  - Should subcomponents always have their own test files? What about type tests and stories?
+- Would this structure help if we wanted to move toward one-package-per-component like react-aria does?


This approach totally opens doors to this and makes codesplitting much easier to maintain. Big ➕

I think that makes sense — personally I don't know enough about what makes tree shaking possible or not possible to be confident that separate packages are actually necessary.

rezrah · 2021-12-02T09:50:52Z

Pretty hyped for this. It'll improve the experience across the board, for both contributors and consumers. Thanks for proposing it @jfuchs.

siddharthkp · 2021-12-06T16:51:17Z

contributor-docs/adrs/adr-XXX-file-structure.md

+  - Should the `index.ts` in each component directory export every subcomponent under its own name, or should we prefer `Object.assign`ing subcomponents to the parent component?
+  - Should subcomponents always have their own test files? What about type tests and stories?
+- Would this structure help if we wanted to move toward one-package-per-component like react-aria does?
+- Could we set a standard for replacement components? E.g., NewButton vs. Button2


my personal vote is for src/drafts/Button or even src/drafts/Button2.

Ohh interesting — can you say more about why you like that?

I hesitate because it'd mean advancing the status of a component from propsed -> alpha would have to be a breaking change, right?

I hesitate because it'd mean advancing the status of a component from proposed -> alpha would have to be a breaking change, right?

Don't think so. we should use drafts for replacement components which can be at a more advanced maturity than their current counterpart. For example, ActionList v1 is alpha but ActionList v2 can be at beta maturity while being in drafts. This is why we chose drafts over other names. More context here: #1609 (comment)

I like it more because it matches the bundle scope. drafts are not part of the main index.js bundle.

siddharthkp

Love it! ship it!

mperrotti · 2021-12-06T22:43:49Z

contributor-docs/adrs/adr-XXX-file-structure.md

+- On subcomponents:
+  - Do subcomponents filenames need to be namespaced with the parent component? (e.g., `BreadCrumbsItem.tsx` vs `Item.tsx`)
+  - Should the `index.ts` in each component directory export every subcomponent under its own name, or should we prefer `Object.assign`ing subcomponents to the parent component?
+  - Should subcomponents always have their own test files? What about type tests and stories?


I don't think so. There are going to be subcomponents that rely on some context to get passed down from the parent.

colebemis · 2021-12-15T20:47:33Z

contributor-docs/adrs/adr-XXX-file-structure.md

+
+- Every component should have its own PascalCased directory directly under `src/`
+- Subcomponents meant to be used as children of their parent component should be properties of the exported component (e.g., `BreadCrumbs.Item`)
+- Subcomponents meant to be used on their own should be exported as a named export (e.g., `ButtonDanger`)


Is this a pattern we want to encourage?

I'd argue that it's not a "subcomponent" if it's exported on its own. If it's not a property of a parent component, I think it should probably have it's own directory.

I like simple rules.... but thinking about that with ButtonDanger.... that doesn't feel like it merits its own component, and I'm not sure I'd want that to just be a prop on Button (i.e., <Button variant="danger"...). Would you say ButtonDanger should have its own directory?

I'm not sure I'd want that to just be a prop on Button (i.e., <Button variant="danger"...)

FYI ButtonDanger is going away in favor of a variant prop. @pksjce's Button2 uses that API.

I don't think we plan to have any components like ButtonDanger in the future

Okay cool. That settles it then :-)

colebemis · 2021-12-15T20:48:33Z

contributor-docs/adrs/adr-XXX-file-structure.md

+### Rules
+
+- Every component should have its own PascalCased directory directly under `src/`
+- Subcomponents meant to be used as children of their parent component should be properties of the exported component (e.g., `BreadCrumbs.Item`)


Does this mean that the Object.assign happens in Breadcrumbs.tsx or index.tsx?

I didn't mean to specify — but if you think this should I'm fine with that!

We can add more rules to this doc as we form opinions. So I guess it's fine to be vague now.

IMO, the Object.assign should happen in Breadcrumbs.tsx. index.tsx seems mostly like an implementation detail to make imports nicer

Should index.tsx export default or named?

Hmm. I lean towards named export. In general, I'm not a big fan of default exports (unless they're necessary for some reason)

contributor-docs/adrs/adr-XXX-file-structure.md

Co-authored-by: Cole Bemis <colebemis@github.com>

contributor-docs/adrs/adr-XXX-file-structure.md

Co-authored-by: Cole Bemis <colebemis@github.com>

* Proposes a new file structure (ADR) * Update contributor-docs/adrs/adr-XXX-file-structure.md Co-authored-by: Cole Bemis <colebemis@github.com> * More questions about the implementation process * Doc updates; removal of open questions * BreadCrumbs -> Breadcrumbs * Remove note about separately exported subcomponents * Update contributor-docs/adrs/adr-XXX-file-structure.md Co-authored-by: Cole Bemis <colebemis@github.com> Co-authored-by: Cole Bemis <colebemis@github.com>

jfuchs added the skip changeset This change does not need a changelog label Dec 1, 2021

colebemis reviewed Dec 1, 2021

View reviewed changes

contributor-docs/adrs/adr-XXX-file-structure.md Outdated Show resolved Hide resolved

github-actions bot deployed to Preview December 1, 2021 21:16 View deployment

pksjce reviewed Dec 1, 2021

View reviewed changes

contributor-docs/adrs/adr-XXX-file-structure.md Outdated Show resolved Hide resolved

github-actions bot deployed to Preview December 1, 2021 22:54 View deployment

rezrah reviewed Dec 2, 2021

View reviewed changes

colebemis added the react label Dec 6, 2021

siddharthkp reviewed Dec 6, 2021

View reviewed changes

siddharthkp approved these changes Dec 6, 2021

View reviewed changes

mperrotti mentioned this pull request Dec 6, 2021

Adds a component to support checkbox and radio groups #1657

Merged

6 tasks

mperrotti reviewed Dec 6, 2021

View reviewed changes

pksjce marked this pull request as ready for review December 9, 2021 06:48

pksjce requested review from a team and mperrotti December 9, 2021 06:48

pksjce marked this pull request as draft December 9, 2021 06:48

pksjce approved these changes Dec 9, 2021

View reviewed changes

github-actions bot deployed to Preview December 15, 2021 17:14 View deployment

jfuchs force-pushed the jfuchs/structure-conventions branch from 210e268 to d902f45 Compare December 15, 2021 17:14

github-actions bot deployed to Preview December 15, 2021 17:14 View deployment

jfuchs marked this pull request as ready for review December 15, 2021 17:31

jfuchs requested review from colebemis and rezrah December 15, 2021 17:31

colebemis reviewed Dec 15, 2021

View reviewed changes

contributor-docs/adrs/adr-XXX-file-structure.md Outdated Show resolved Hide resolved

github-actions bot deployed to Preview December 15, 2021 21:00 View deployment

jfuchs and others added 6 commits December 15, 2021 14:24

Proposes a new file structure (ADR)

d185179

Update contributor-docs/adrs/adr-XXX-file-structure.md

493c954

Co-authored-by: Cole Bemis <colebemis@github.com>

More questions about the implementation process

117039b

Doc updates; removal of open questions

09edf80

BreadCrumbs -> Breadcrumbs

0a4a2b7

Remove note about separately exported subcomponents

6696d1e

jfuchs force-pushed the jfuchs/structure-conventions branch from 04d6620 to 6696d1e Compare December 15, 2021 22:24

github-actions bot deployed to Preview December 15, 2021 22:24 View deployment

jfuchs requested a review from colebemis December 15, 2021 22:43

colebemis reviewed Dec 16, 2021

View reviewed changes

contributor-docs/adrs/adr-XXX-file-structure.md Outdated Show resolved Hide resolved

colebemis approved these changes Dec 16, 2021

View reviewed changes

jfuchs and others added 2 commits December 16, 2021 14:50

Update contributor-docs/adrs/adr-XXX-file-structure.md

421be43

Co-authored-by: Cole Bemis <colebemis@github.com>

Merge branch 'main' into jfuchs/structure-conventions

f252635

github-actions bot deployed to Preview December 16, 2021 22:50 View deployment

github-actions bot deployed to Preview December 16, 2021 22:51 View deployment

jfuchs merged commit b05472e into main Dec 16, 2021

jfuchs deleted the jfuchs/structure-conventions branch December 16, 2021 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propose a new file structure (ADR) #1678

Propose a new file structure (ADR) #1678

jfuchs commented Dec 1, 2021

changeset-bot bot commented Dec 1, 2021 •

edited

Loading

github-actions bot commented Dec 1, 2021 •

edited

Loading

colebemis commented Dec 1, 2021

rezrah Dec 2, 2021

siddharthkp Dec 6, 2021

mperrotti Dec 16, 2021

rezrah Dec 2, 2021

jfuchs Dec 3, 2021

rezrah Dec 2, 2021

jfuchs Dec 3, 2021

rezrah commented Dec 2, 2021

siddharthkp Dec 6, 2021

jfuchs Dec 6, 2021

siddharthkp Dec 6, 2021 •

edited

Loading

siddharthkp left a comment

mperrotti Dec 6, 2021

colebemis Dec 15, 2021

jfuchs Dec 15, 2021

colebemis Dec 15, 2021 •

edited

Loading

jfuchs Dec 15, 2021

colebemis Dec 15, 2021

jfuchs Dec 15, 2021

colebemis Dec 15, 2021

jfuchs Dec 15, 2021

colebemis Dec 16, 2021

Propose a new file structure (ADR) #1678

Propose a new file structure (ADR) #1678

Conversation

jfuchs commented Dec 1, 2021

Merge checklist

changeset-bot bot commented Dec 1, 2021 • edited Loading

⚠️ No Changeset found

github-actions bot commented Dec 1, 2021 • edited Loading

size-limit report 📦

colebemis commented Dec 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rezrah commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthkp Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

siddharthkp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colebemis Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Dec 1, 2021 •

edited

Loading

github-actions bot commented Dec 1, 2021 •

edited

Loading

siddharthkp Dec 6, 2021 •

edited

Loading

colebemis Dec 15, 2021 •

edited

Loading