docs(rfc): create file naming guidelines for convergence#16870
docs(rfc): create file naming guidelines for convergence#16870Hotell wants to merge 4 commits intomicrosoft:masterfrom
Conversation
| - some files are PascalCased, some camelCased | ||
| - not explicitly communicating the intent | ||
| - what does `use` mean ? | ||
| - seasoned react dev will probably guess that it's a hook. Thing is, that this file might export more that 1 hook. |
| <!-- This is the bulk of the RFC. Explain the proposal or design in enough detail for the inteded audience to understand. --> | ||
|
|
||
| - use consistent names for all files. | ||
| - follow a pattern that describes the symbol's feature or the file scope(component name) then its type. |
There was a problem hiding this comment.
| - follow a pattern that describes the symbol's feature or the file scope(component name) then its type. | |
| - Follow a pattern that describes the symbol's feature or the file scope (component name) followed by its type. |
| | Current File Name | Proposed File name | Symbol Name | Remarks | | ||
| | ------------------- | ------------------- | ----------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | index.tsx | index.ts | | No changes from current | | ||
| | Example.tsx | example.ts | `export const Example = () => {..}` | To enforce rendering logic only within `.view.` type file/s. TS will enforce this by default. Not extremely picky on this one, we can use `.tsx` as well to match extension with test/story - priority is to be consistent | |
There was a problem hiding this comment.
Personally, I find the naming Example.tsx to be intuitive since it tells me we're exporting an object/component/etc. In fact, this is how the naming convention works in Zig as well for instance - if a module is in fact a struct, then name it as capital such as Example; otherwise, lowercase.
There was a problem hiding this comment.
I hear you. Just note that this is primarily about having consistency.
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 0dbe8a2d955f2a9a589aef6797654a94d53bcc4f (build) |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 8888008:
|
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
| | Current File Name | Proposed File name | Symbol Name | Remarks | | ||
| | ------------------- | ------------------- | ----------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | index.tsx | index.ts | | No changes from current | | ||
| | Example.tsx | example.ts | `export const Example = () => {..}` | To enforce rendering logic only within `.view.` type file/s. TS will enforce this by default. Not extremely picky on this one, we can use `.tsx` as well to match extension with test/story - priority is to be consistent | |
There was a problem hiding this comment.
| | Example.tsx | example.ts | `export const Example = () => {..}` | To enforce rendering logic only within `.view.` type file/s. TS will enforce this by default. Not extremely picky on this one, we can use `.tsx` as well to match extension with test/story - priority is to be consistent | | |
| | Example.tsx | Example.ts | `export const Example = () => {..}` | |
- it's a common pattern to use Pascal Case for components: https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-pascal-case.md
- it's a common pattern to match filename to default export:
- https://github.com/mui-org/material-ui/tree/next/packages/material-ui/src/Accordion
- https://github.com/adobe/react-spectrum/tree/main/packages/%40react-spectrum/alert/src
- https://github.com/reakit/reakit/tree/master/packages/reakit/src/Box
- https://github.com/Semantic-Org/Semantic-UI-React/tree/master/src/elements/Button
- https://github.com/segmentio/evergreen/tree/master/src/avatar/src
- https://github.com/formium/formik/tree/master/packages/formik/src
- React team uses Pascal Case in docs https://reactjs.org/docs/code-splitting.html#reactlazy
There was a problem hiding this comment.
I provided explicit reasoning in this document. This kind of feedback is not very helpful TBH.
- we are not react team
- default exports are an anti-pattern (nor is fluent using that ECMA feature)
- we can start passing links indefinitely, to showcase who uses what :)
There was a problem hiding this comment.
I agree with Shift on this one, I'd prefer to keep the uppercase filename basically for the reasons he listed.
- Clearly indicates that this file exports a component (even if it doesn't contain any rendering logic)
- Matches the name of the main/only export (not relevant for this purpose whether that export is default or named)
- Matches the convention used by both v8 and v0 today, and is extremely common throughout the community. Even though nobody has declared this as the "official" convention, at least from briefly looking through other React component repos, it's harder to find a repo that doesn't use uppercase names for component files than one that does.
| | ------------------- | ------------------- | ----------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | index.tsx | index.ts | | No changes from current | | ||
| | Example.tsx | example.ts | `export const Example = () => {..}` | To enforce rendering logic only within `.view.` type file/s. TS will enforce this by default. Not extremely picky on this one, we can use `.tsx` as well to match extension with test/story - priority is to be consistent | | ||
| | Example.test.tsx | example.spec.tsx | `describe->it` | Integration test on JSDOM level (jest) for whole component | |
There was a problem hiding this comment.
| | Example.test.tsx | example.spec.tsx | `describe->it` | Integration test on JSDOM level (jest) for whole component | | |
| | Example.test.tsx | Example.spec.tsx | `describe->it` | Integration test on JSDOM level (jest) for whole component | |
If we will go with Pascal Case, this should be also in Pascal Case: https://github.com/microsoft/fluentui/pull/16870/files#r574385390
| | Example.test.tsx | example.spec.tsx | `describe->it` | Integration test on JSDOM level (jest) for whole component | | ||
| | storybook (NONE) | example.stories.tsx | `export const Example = () => {..}` | Specific file type for Storybook consumption. Main "starting" point for inner loop - daily development. | | ||
| | mdx (NONE) | example.stories.mdx | --- | Markdown(MDX) for consumer facing documentation(mainly for storybook at the moment, but can be transformed to any other tool that can parse MDX). | | ||
| | renderExample.tsx | example.view.tsx | `export const render = () => {..}` | `.render` might work as well, to match symbol name. | |
There was a problem hiding this comment.
A function should not be named as render, it should be renderComponentName as we should avoid collisions in consumer imports. For example:
import { render } from 'react-dom'
import { render } from '@fluentui/react-button'
import { render } from '@fluentui/react-menu'It also breaks DX on our side as if search will be used how many matches for render you will get?
There was a problem hiding this comment.
For me, it's confusing to name files as example.view.tsx when a single export is renderExample function. It's not a common thing in React community:
There was a problem hiding this comment.
renderComponentName
That's a typo, thx.
A function should not be named as
render, it should berenderComponentNameas we should avoid collisions in consumer imports. For example:
FWIW that's not true :) consumer can name imported symbols anyhow he wants.
It also breaks DX on our side as if search will be used how many matches for render you will get?
maybe out of topic but my understanding was that those renderers are meant to be used solely per package/component
There was a problem hiding this comment.
"consumer can name imported symbols anyhow he wants." - from my experience with navigating relatively large code bases - the more specificity you have in names and the more consistency there is between file and export names, the better - because then you can easily combine fulltext search and file name search
In 99% of the cases probably yes, renderers will be used only in the package and component. But if the consumer wants to extend the component and add/replace hooks in it, he can still import and reuse the render function.
| | storybook (NONE) | example.stories.tsx | `export const Example = () => {..}` | Specific file type for Storybook consumption. Main "starting" point for inner loop - daily development. | | ||
| | mdx (NONE) | example.stories.mdx | --- | Markdown(MDX) for consumer facing documentation(mainly for storybook at the moment, but can be transformed to any other tool that can parse MDX). | | ||
| | renderExample.tsx | example.view.tsx | `export const render = () => {..}` | `.render` might work as well, to match symbol name. | | ||
| | useExample.ts | example.hook.ts | `export const useExample = () => {}` | main logic encapsulated under hook -> name should be the same as encapsulated component (in our case `example`) | |
There was a problem hiding this comment.
This breaks common convention in React community:
There was a problem hiding this comment.
I'd just add that there are no existing conventions ratified in any kind of official community style-guide, nor in fluent ui org :) that's the primary aim of this RFC -> focus/ratification on consistency.
There was a problem hiding this comment.
That's fair, I used these repos as a reference to show a common pattern. Fluent's repo follows it, too, currently.
Another fair note will be, that there is an explicit naming convention for hooks, they should start with use*() keyword.
| **Why?** | ||
|
|
||
| - kebab case is more human readable especially in longer words | ||
| - mitigates issues with git on various OS/case in/consistent files systems (eg. rename `helloWorld` to `helloworld`, commit, push, merge... now someone wants to change it back to `helloWorld`... ) |
There was a problem hiding this comment.
This is handled by TypeScript compiler via forceConsistentCasingInFileNames (https://www.typescriptlang.org/docs/handbook/compiler-options.html) and is enabled across the repo. Is it a real issue currently?
|
|
||
| --- | ||
|
|
||
| - use `.tsx` only for files that contain view/rendering related logic |
There was a problem hiding this comment.
| - use `.tsx` only for files that contain view/rendering related logic | |
| - use `.tsx` only for files that contain view/rendering related logic | |
| - enable lint rule that will enforce one way |
I suggest to add into proposal an ESLint rule for this, otherwise it's easy to fail this convention. There is jsx-filename-extension, but I have not seen a similar for TS files:
- no JSX in files - use
.ts - JSX in files - use
.tsx(this is forced by compiler anyway)
|
Likely move to March |
|
Status update:
March goal: merge or not :) |
|
Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes. The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code. |
msft-fluent-ui-bot
added
the
Resolution: Soft Close
ecraig12345
removed
the
Resolution: Soft Close
📊 Bundle size reportUnchanged fixtures
|
|
Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes. The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code. |
msft-fluent-ui-bot
added
the
Resolution: Soft Close
10 participants
Description of changes
Based on #16806, this RFC attempts to implement consistency for naming converged packages files with proper intent.
NOTE:
Focus areas to test
(optional)