Skip to content
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

RFC: communicating package convergence status #16772

Closed
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 141 additions & 0 deletions rfcs/packaging/communicating-convergence-status.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
# RFC: Communicating convergence status for `@fluentui/*` packages (and related code moves)

**Note that this RFC focuses solely on near-term steps to clearly communicate to consumers which of our packages are old vs. converged. It's focused primarily on v7/8 and converged packages and does not deal with folder structure or northstar packages.**

**[Related RFC around other package structure issues during convergence](https://github.com/microsoft/fluentui/pull/16577)**

Contributors: @paulgildea, @JustSlone, @ecraig12345, @khmakoto

## Summary

Currently, we have a lot of different component packages, and their purpose, production-readiness, and convergence status are not clear to consumers or contributors.

There are a few aspects to the proposed solution, involving a mix of code moves and documentation updates:

- "Converged" component packages should only contain convergence work
- _(done)_ ~~For `react-{checkbox,link,slider,tabs,toggle}`:~~
- ~~Move old (v8) component implementations back to `react-internal`~~
- ~~Move prototype "converged" versions to the package roots and get rid of `/next` folders~~
- Get rid of `react-internal`
- `react-internal` components move back to `react`
- To break a cycle: `react-date-time` components move into `react`, and `react-date-time` re-exports them
- Open question: should we make code location or naming changes to any other `react-*` packages which still contain legacy components?
Copy link
Collaborator

@JustSlone JustSlone Feb 5, 2021

Choose a reason for hiding this comment

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

I would say it would be ideal if the only react-* packages were converged components, but I don't think it's a high priority. I would certainly not suggest we delay v8 release for moving additional react-* packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Keep this because we will have a variety of other platforms (inside and outside our repo) under @fluentui scope.

Would be nice if it could mean "converged" but that's not realistic.

- Add badges to package readmes to clarify GA/preview/dev status

## Problem statement

Currently, we have a lot of different component packages, and their purpose, production-readiness, and convergence status are not clear to consumers or contributors.

### Background: current packages

(See also https://github.com/microsoft/fluentui/wiki/Repo-structure)

<!-- prettier-ignore-start -->
| Package | Notes about current implementation |
|---------|-------|
| `@fluentui/react` | Exports official finished components which we plan to support long-term (or provide good migration paths for major changes). Will be fixed after this proposal: ~~Mostly re-exports other packages, but a few components live here to avoid circular deps.~~ |
| ~~`@fluentui/react-internal`~~ | Going away after this proposal. ~~Split out solely to work around circular dep issues, and most of the code for old components lives here. Nobody should reference it directly outside our repo. `@fluentui/react` re-exports everything from here, and some of the `@fluentui/react-<component>` packages depend on this package.~~ |
| `@fluentui/react-button` | Was previously considered ready for release (and therefore ready to export from `@fluentui/react`), but we've decided that while it's ready for partners to try out and should be stable within this major release, there will be some significant changes in the next major release. |
| `@fluentui/react-cards` | Weird case since there's an old version (built on `@uifabric/foundation` `createComponent` patterns) but it never officially graduated from experimental status and isn't exported from the suite. It also has a `next` folder where the converged version is currently being worked on. |
| `@fluentui/react-date-time` | Previously `@uifabric/date-time`. It's a rewrite (mostly by OWA folks, started awhile ago) of the original OUFR Calendar and DatePicker, but still using older patterns. New for v8, the Calendar and DatePicker from this package are exported from `@fluentui/react` by default. |
| `@fluentui/react-focus` | Current version of FocusZone with most of the work needed to converge with v0's FocusZone. Still differs in are some default values and things that are pulled from the theme. |
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
| `@fluentui/react-focus` | Current version of FocusZone with most of the work needed to converge with v0's FocusZone. Still differs in are some default values and things that are pulled from the theme. |
| `@fluentui/react-focus` | Current version of FocusZone with most of the work needed to converge with v0's FocusZone. Still differs in some default values and things that are pulled from the theme. |

| `@fluentui/react-checkbox`<br/>`@fluentui/react-link`<br/>`@fluentui/react-slider`<br/>`@fluentui/react-toggle`<br/>`@fluentui/react-tabs` | These previously default-exported the non-converged (v8) versions of the respective components (with prototype converged versions under `src/next`). The v8 components were recently moved backed to the suite and the convergence work was moved to the root. |
| `@fluentui/react-avatar`<br/>`@fluentui/react-flex`<br/>`@fluentui/react-image`<br/>`@fluentui/react-text` | These only contain work on the converged component versions (in varying states of active work and completeness). |
| `@fluentui/react-charting` | Has active work ongoing but is owned by a different team and doesn't really follow any of the newer patterns we're working on. (Once we establish a pattern for "high-value component" repos, we may split this out.) |
| `@fluentui/react-experiments` | Weird legacy package which we don't have a good story for moving forward, and probably don't recommend (certainly don't promote) for use except by authoring teams of the individual components. It's a mix of components in active development or heavy use by partners, old trials of previous patterns we should definitely delete, and things of unknown status or abandoned. [More discussion here.](https://github.com/microsoft/fluentui/issues/16452) |
<!-- prettier-ignore-end -->

## Detailed Design or Proposal

### All v8 components live in `@fluentui/react`

Motivation: improve contribution story, in some ways improve consumption story.

- Get rid of `react-internal` _(see detailed discussion below)_
- Move all components back into `react`
- To break a cycle: move `react-date-time` components into `react`, and have `react-date-time` re-export them
- _(done)_ ~~Move v8 Checkbox, Link, Pivot, Slider, Toggle out of component packages and back to `react`~~
- _(done)_ ~~`react-{checkbox,link,slider,tabs,toggle}`: Move prototype "converged" components in into the package roots and get rid of `/next` folders.~~
- The component packages can _temporarily_ depend on `react` if needed for shared implementation parts.
- Before release (ideally ASAP), any deps on `react` would need to be removed and handled with slots intead: e.g. if we haven't gotten to converge Label yet, converged Checkbox by default has built-in `<label>` as its `label` slot. But `@fluentui/react` will define and export a version of Checkbox which uses the old Label.
- _(done)_ ~~Delete react-next: it's not being used, causing minor confusion, and can come back later without too much trouble.~~

#### Open question: any other code moves?

See open questions section.

#### Decision to remove `react-internal`

`@fluentui/react-internal` was created as a workound for circular dependency issues that arose when breaking out components from the `@fluentui/react` version 8 suite package into individual packages (`react-checkbox` etc).

It broke the cycles but left a very convoluted developer experience for v8 components: the majority lived in `react-internal`, a handful lived in individual component packages like `react-checkbox`, and a handful which used components from the individual packages had to live in `react`. From the perspective of anyone but ~4 team members who worked on the split, there was no rhyme or reason for what lived where, and building/testing became more painful.

However, more recent developments have invalidated most of `react-internal`'s reasons for existence:

- We decided individual component packages like `react-checkbox` should _only_ contain converged components, and moved the v8 versions back to the suite (eliminating most current cycles)
- Exception: `react-date-time` (not at all converged) is exported by the suite as of version 8, creating a cycle, but we can break this cycle as described above
- Shift in focus to aggressively converging components, with a hard policy that they _must not_ depend on old components from either v8 or v0 (eliminating most risk of future cycles)

While getting rid of `react-internal` is extra work, it seems worthwhile to get rid of the technical debt and improve the dev experience.

### Open question: should `@fluentui/react-*` mean converged? (remove `react-` prefix from old packages?)

See discussion later.

### Add status badges to all component package readmes

The badges below are generated using https://shields.io/ and link to a placeholder wiki page which will later contain explanations of each status.

We definitely want badges for stability, possibly with sub-badges for convergence status:

- [![in development](https://img.shields.io/badge/stability-in%20development-orange)](https://github.com/microsoft/fluentui/wiki/Package-status)
- [![preview](https://img.shields.io/badge/stability-preview-yellow)](https://github.com/microsoft/fluentui/wiki/Package-status)
- Possible sub-badge: [![preview (legacy)](<https://img.shields.io/badge/stability-preview%20(legacy)-yellow>)](https://github.com/microsoft/fluentui/wiki/Package-status)
- [![stable](https://img.shields.io/badge/stability-stable-green)](https://github.com/microsoft/fluentui/wiki/Package-status)
- Possible sub-badge: [![stable (legacy)](<https://img.shields.io/badge/stability-stable%20(legacy)-green>)](https://github.com/microsoft/fluentui/wiki/Package-status)
- [![deprecated](https://img.shields.io/badge/stability-deprecated-red)](https://github.com/microsoft/fluentui/wiki/Package-status)

We might also want separate badges for category/convergence/??? (naming suggestions welcome)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer the separate badges for convergence (if we need them).

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I'd add explicit annotations to public APIS as well - ideally have automation that would take that as source of truth and generate those badges.

eg:

/**
 * @experimental
 */
export function Button(props){....}

/**
 * @stable
 */
export function Avatar(props){....}

Copy link
Member Author

Choose a reason for hiding this comment

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

I was going to suggest using the @packageDocumentation comment for this purpose and including a status annotation in the comment, but at least according to the API Extractor docs that's explicitly not supported. 🙄

On individual APIs we could use release tags but that has to be done on each API individually. While this is definitely something we should consider using, I'm not sure offhand how you'd set up automation to determine the overall release status of a particular package based on individual API tags.

Another possibility for communicating release status is to just use semver, like staying on a prerelease (-dev followed by -beta?) until it's ready. I guess in theory 0.x is also supposed to communicate this but it's a bit less explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah those annotations will definitely not "solve" the general status. but it's always a good idea to include them for particular apis to explicitly communicate.

I'm all for semver and prerelase tags 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

We may be too quick to "release" unstable/semi-stable things to partners--okay during iteration but we should keep the heaviest churn internal.

@alpha/@beta tags on individual APIs could be good if we have unstable APIs within an overall stable package, but having to add annotations on every API in a new package is less useful.

Copy link
Member Author

@ecraig12345 ecraig12345 Feb 11, 2021

Choose a reason for hiding this comment

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

Should ensure that before we "ship" anything, need agreement from relevant parties on both sub-teams. Don't repeat what we did with react-compose where we release too soon and not all parties are aware.

Consider quarterly planning for breaks and new releases for converged stuff only (separate RFC?). Won't affect v0 or v8.

Copy link
Member Author

Choose a reason for hiding this comment

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

Make an RFC or something discussing previous issues we hit with versioning a suite package.


- [![legacy](https://img.shields.io/badge/convergence-legacy-lightgrey)](https://github.com/microsoft/fluentui/wiki/Package-status)
- [![mixed](https://img.shields.io/badge/convergence-mixed-yellow)](https://github.com/microsoft/fluentui/wiki/Package-status)
- [![done](https://img.shields.io/badge/convergence-done-green)](https://github.com/microsoft/fluentui/wiki/Package-status)

Notes:

- Charting and maybe experiments would probably fall into the "preview (legacy)" category
- Suite packages should list which components are converged or not

## Discarded Solutions

<!-- As you enumerate possible solutions, try to keep track of the discarded ones. This should include why we discarded the solution. -->

## Open Issues

<!-- Optional section, but useful for first drafts. Use this section to track open issues on unanswered questions regarding the design or proposal. -->

### Should we change anything about `react-cards`?
Copy link
Member Author

Choose a reason for hiding this comment

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

@khmakoto I'd appreciate your input on this section in particular

Copy link
Member

Choose a reason for hiding this comment

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

I think we should delete it from version 8. To my understanding there was one partner using it and they knew it wasn't officially released. We could still provide support in the 7.0 branch and, honestly, there's not much in terms of influx of Card related issues.


The Card exported by default from `react-cards` is not converged and was never quite officially released, and there's work on a converged version in `src/next`. I'm not sure how many people were using the old Card.

Should we move the old implementation elsewhere (to experiments maybe?) or delete it so the new one can be exported by default? If we do that, what happens to existing consumers?

Another sneaky way out would be moving the new card to a new package `react-card` (singular), which is a more standard name but could definitely lead to confusion having both.

### Should `@fluentui/react-*` mean converged? (remove `react-` prefix from old packages?)
Copy link
Collaborator

@JustSlone JustSlone Feb 5, 2021

Choose a reason for hiding this comment

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

Wasn't part of the reason to prefix with react- to make it clear these packages were associated with @fluentui/react instead of northstar or web-components? Would we be losing that if we go back?

I don't have a strong preference, but wanted to make sure we aren't forgetting the reasons we prefixed with react-.

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion:

I'd go with complete isolated approach in terms of folder structure (if we don't wanna do it in new repo/next branch - very common in OSS)

/packages
  /react-next (>= v9)
     /react     - `name: @fluentui/react; version: 9.0.0-alpha.1`
          -> ```
                bundle which re-exports everything 
                export * from '@fluentui/react-button'
                export * from '@fluentui/react-avatar'
              ....
       ```
     /button - `name: @fluentui/react-button; version: 9.0.0-alpha.1`
     /avatar- `name: @fluentui/react-avatar; version: 9.0.0-alpha.1`
     /text- `name: @fluentui/react-text; version: 9.0.0-alpha.1`
     /theme- `name: @fluentui/theme; version: 9.0.0-alpha.1`
     /theme-provider - `name: @fluentui/theme-provider; version: 9.0.0-alpha.1`
  /react (v8)
  /react-button (v8)
  /react-avatar (v8)
  /fluentui (v0)
  /web-components

Copy link
Contributor

Choose a reason for hiding this comment

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

hah just realised that this wouldn't allow users to use both 8/next in hybrid mode ...

Anyways I'd still keep it "isolated" from folder structure perspective:

question/thought:

What about following?:

  • consumers are using v7/8
  • they have in their deps(package.json) "@fluentui/react": "7.x"
  • we have converged Button -> publish "@fluentui/react-button": "9.0.0-alpha.1"
  • consumer want to use converged button
  • he adds "@fluentui/react-button": "9.0.0-alpha.1"
  • now he can use both buttons:
import {Button} from '@fluentui/react'
import {Button as ConvergedButton} from '@fluentui/react-button'
  • convergence continues
  • some initial feature set of components is converged
  • we publish - "@fluentui/react": "9.0.0-alpha.1", "dist-tag": "next"
  • consumer continues migration/hybrid mode
  • consumer comes to state that he is not using any v7/8 anymore (convergence continues)
  • we have new converged components -> publish - "@fluentui/react": "9.0.0-alpha.2", "dist-tag": "next"
  • consumer installs new converged suite, and removes atomic converged packages
-"@fluentui/react": "7.x"
+"@fluentui/react": "9.0.0-alpha.2"
-"@fluentui/react-button": "9.0.0-alpha.2"
-"@fluentui/react-avatar": "9.0.0-alpha.2"
import {Button} from '@fluentui/react'
- import {Button as ConvergedButton} from '@fluentui/react-button'
  • initial convergence feature set is stable
  • we release "@fluentui/react": "9.0.0", "dist-tag": "latest"

Summary:

Pros:

  • no need for temporary name for fluentui react barrel
  • "less" confusion to consumers what is what
  • explicit folder and single source of truth for devs working on convergence

Cons:

  • Until consumer have not fully migrated to v9 they won't be able to use just whole barrel rather than granular installs.
    • I wouldn't say it's a con necessarily rather than more "verbose" syntax to consumers

WDYT?

Copy link
Member Author

@ecraig12345 ecraig12345 Feb 5, 2021

Choose a reason for hiding this comment

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

The reasoning we went with at the time of the name changes was literally "any package which uses React should have react- prefix" (not association with a particular parent package). But we can revisit that reasoning if it makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Hotell I think that will work at the broad strokes level.
Here's some useful context from later last year: [RFC] Increasing focus and iteration speed in convergence - Lean the Towers v2.0

To summarize what I think will work, and what I think you're saying here

  • Ship react-button 9.0-alpha (could it also be react-button 0.0.x-alpha?)
  • Customers use v7/v8/v0 and [email protected]
  • Later we roll react-button into @fluentui/[email protected]
    • Addition: when we do this I think we need to update components in @fluentui/react that use button to use react-button 9.0
  • Eventually @fluentui/react is mostly converged components
    • At this point we could choose one of this options:
      • Make minimal updates to legacy components to use tokens/styling making them "converged enough"
      • Spin out remaining legacy components to their own packages, making @fluentui/react 100% converged
      • Some other great idea we come up with later

Regarding:

/packages/react-next/button (>= v9)

react-next is a folder name that will only be accurate for a period of time, which means we'll have to move the components out of this folder at some point. Do we really want to force ourselves to do file moves in the repo? I'd recommend we name this something more durable such as: react-components which could eventually become a place where we put all the component packages for the @fluentui/react library. We could then re-export these wherever we need to.

Another question on packages/react-next/button wouldn't this make it harder to find the folder which exports @fluentui/react-button? I think that's one of the benefits of having a flat packages/* folder there is a 1:1 mapping between released packages and folders in the repo. I could be wrong though.

Regarding:

consumer comes to state that he is not using any v7/8 anymore (convergence continues)

The point at which consumers are not using any v7/v8/v0 components is probably a long distance in the future. The goal for now is to build out a baseline set of converged components such that the converged components cover the common components needed to build an application. While the desire is to eventually deprecate all 80 legacy components, the time frame on that is likely exceedingly long, and the number of technical challenges that must be solved to do that is daunting.

Another thing to consider is that the cost of doing a major upgrade of @fluentui/react for both us and our partners (though mostly partners) is fairly high. I would suggest we plan on v9.0 being much later and mix-matching the converged packages with both @fluentui/[email protected] and @fluentui/react-northstar for some time.

I honestly think it wouldn't be a bad idea to keep our options open about how specifically components fold back into @fluentui/react (as long as we have multiple workable options). The number one priority we have right now is getting to a useful set of baseline components. We have partners looking to build entirely on top of converged components right now. This is a great opportunity to test out our converged model in real use cases and iterate on it quickly. Folding back into @fluentui/react is of less importance right now. As we go forward we will learn more which likely will lead us to a particular implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another question on packages/react-next/button wouldn't this make it harder to find the folder which exports @fluentui/react-button?

I don't think so:

  • you can directly point user from npm overview to implementation in any particular folder
  • whole convergence is gonna be powered by TS path aliases, one click away to be redirected to right direction
  • the sandboxing/separation is explicit by next folder

With that a question may arise:

  • So how will we able to "gradually" add converged component to "v8" react suite? -> Well only option that I see here is to specify it within direct dependencies -> thus consuming it from npm (as we'll have different versions of same pkg name in same branch) or leveraging "old" build upfront. I'll need to investigate this when the right time comes. (other thing that comes to my mind is to extend react tsconfig with new tsconifg.base.json ...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think so:

  • you can directly point user from npm overview to implementation in any particular folder
  • whole convergence is gonna be powered by TS path aliases, one click away to be redirected to right direction
  • the sandboxing/separation is explicit by next folder

Sounds good. I can see how a subfolder to organize things would go a long way to cleaning things up (just today someone contributing changes to slider changed packages/react-slider rather than packages/react-internal/src/components/slider, I'm not surprised they fell into that trap).

Copy link
Member Author

Choose a reason for hiding this comment

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

For purposes of reducing further scope creep of this RFC, I think any repo structure changes like subfolders that don't affect package naming should be considered separately (I tried to call this out at the beginning of the doc). Totally agree that it's a possible helpful tool for clarifying to contributors what's what, but it's a whole separate discussion for another RFC. Also it can be changed at any time without impact on consumers, so it doesn't block version 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

The naming of a future suite package for consuming only converged components is also intended to be covered by #16577, not this RFC.


For communication purposes to consumers, I'm not sure that renaming all our React-based packages to have the `react-` prefix (which we're using for converged packages) was the right thing to do, since it worsens confusion about which packages are converged.

While the status badges proposed above would help clarify this, it might also be worth reconsidering some of the new package names before we release version 8 and are stuck with the names for awhile.

Some packages with this issue:

| Package | Old name | Notes |
| -------------------------- | ------------------ | -------------------------------------------------------- |
| `react-cards` | same | see above |
| `react-charting` | `charting` | no plan to converge |
| `react-date-time` | `date-time` | exported from FUIR v8 |
| `react-docsite-components` | `example-app-base` | legacy doc site stuff, no plan to converge |
| `react-experiments` | `experiments` | random mix of active/dead |
| `react-file-type-icons` | `file-type-icons` | |
| `react-monaco-editor` | `tsx-editor` | react wrapper for monaco editor, probably won't converge |