Update Coding Guidelines (#441)
Co-authored-by: Christopher Anderson <anderson.chris.john@gmail.com>
This commit is contained in:
parent
1ec0d9a0be
commit
1f4074f3e8
|
@ -0,0 +1,194 @@
|
|||
# Coding Guidelines and Recommendations
|
||||
|
||||
Cosmos Explorer has been under constant development for over 5 years. As a result, there are many different patterns and practices in the codebase. This document serves as a guide to how we write code and helps avoid propagating practices which are no longer preferred. Each requirement in this document is labeled and color-coded to show the relative importance. In order from highest to lowest importance:
|
||||
|
||||
✅ DO this. If you feel you need an exception, engage with the project owners _prior_ to implementation.
|
||||
|
||||
⛔️ DO NOT do this. If you feel you need an exception, engage with the project owners _prior_ to implementation.
|
||||
|
||||
☑️ YOU SHOULD strongly consider this but it is not a requirement. If not following this advice, please comment code with why and proactively begin a discussion as part of the PR process.
|
||||
|
||||
⚠️ YOU SHOULD NOT strongly consider not doing this. If not following this advice, please comment code with why and proactively begin a discussion as part of the PR process.
|
||||
|
||||
💭 YOU MAY consider this advice if appropriate to your situation. Other team members may comment on this as part of PR review, but there is no need to be proactive.
|
||||
|
||||
## Development Environment
|
||||
|
||||
☑️ YOU SHOULD
|
||||
|
||||
- Use VSCode and install the following extensions. This setup will catch most linting/formatting/type errors as you develop:
|
||||
- [Prettier](https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode)
|
||||
- [ESLint](https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint)
|
||||
|
||||
💭 YOU MAY
|
||||
|
||||
- Use the [GitHub CLI](https://cli.github.com/). It has helpful workflows for submitting PRs as well as for checking out other team member's PRs.
|
||||
- Use Windows, Linux (including WSL), or OSX. We have team members developing on all three environments.
|
||||
|
||||
✅ DO
|
||||
|
||||
- Maintain cross-platform compatibility when modifying any engineering or build systems
|
||||
|
||||
## Code Formatting
|
||||
|
||||
✅ DO
|
||||
|
||||
- Use [Prettier](https://prettier.io/) to format your code
|
||||
- This will occur automatically if using the recommended editor setup
|
||||
- `npm run format` will also format code
|
||||
|
||||
## Linting
|
||||
|
||||
✅ DO
|
||||
|
||||
- Use [ESLint](https://eslint.org/) to check for code errors.
|
||||
- This will occur automatically if using the recommended editor setup
|
||||
- `npm run lint` will also check for linting errors
|
||||
|
||||
💭 YOU MAY
|
||||
|
||||
- Consider adding new lint rules.
|
||||
- If you find yourself performing "nits" as part of PR review, consider adding a lint rule that will automatically catch the error in the future
|
||||
|
||||
⚠️ YOU SHOULD NOT
|
||||
|
||||
- Disable lint rules
|
||||
- Lint rules exist as guidance and to catch common mistakes
|
||||
- You will find places we disable specific lint rules however it should be exceptional.
|
||||
- If a rule does need to be disabled, prefer disabling a specific line instead of the entire file.
|
||||
|
||||
⛔️ DO NOT
|
||||
|
||||
- Add [TSLint](https://palantir.github.io/tslint/) rules
|
||||
- TSLint has been deprecated and is on track to be removed
|
||||
- Always prefer ESLint rules
|
||||
|
||||
## UI Components
|
||||
|
||||
☑️ YOU SHOULD
|
||||
|
||||
- Write new components using [React](https://reactjs.org/). We are actively migrating Cosmos Explorer off of [Knockout](https://knockoutjs.com/).
|
||||
- Use [Fluent](https://developer.microsoft.com/en-us/fluentui#/) components.
|
||||
- Fluent components are designed to be highly accessible and composable
|
||||
- Using Fluent allows us to build upon the work of the Fluent team and leads to a lower total cost of ownership for UI code
|
||||
|
||||
### React
|
||||
|
||||
☑️ YOU SHOULD
|
||||
|
||||
- Use pure functional components when no state is required
|
||||
|
||||
💭 YOU MAY
|
||||
|
||||
- Use functional (hooks) or class components
|
||||
- The project contains examples of both
|
||||
- Neither is strongly preferred at this time
|
||||
|
||||
⛔️ DO NOT
|
||||
|
||||
- Use inheritance for sharing component behavior.
|
||||
- React documentation covers this topic in detail https://reactjs.org/docs/composition-vs-inheritance.html
|
||||
- Suffix your file or component name with "Component"
|
||||
- Even though the code has examples of it, we are ending the practice.
|
||||
|
||||
## Libraries
|
||||
|
||||
⚠️ YOU SHOULD NOT
|
||||
|
||||
- Add new libraries to package.json.
|
||||
- Adding libraries may bring in code that explodes the bundled size or attempts to run NodeJS code in the browser
|
||||
- Consult with project owners for help with library selection if one is needed
|
||||
|
||||
⛔️ DO NOT
|
||||
|
||||
- Use underscore.js
|
||||
- Much of this library is now native to JS and will be automatically transpiled
|
||||
- Use jQuery
|
||||
- Much of this library is not native to the DOM.
|
||||
- We are planning to remove it
|
||||
|
||||
## Testing
|
||||
|
||||
⛔️ DO NOT
|
||||
|
||||
- Decrease test coverage
|
||||
- Unit/Functional test coverage is checked as part of the CI process
|
||||
|
||||
### Unit Tests
|
||||
|
||||
✅ DO
|
||||
|
||||
- Write unit tests for non-UI and utility code.
|
||||
- Write your tests using [Jest](https://jestjs.io/)
|
||||
|
||||
☑️ YOU SHOULD
|
||||
|
||||
- Abstract non-UI and utility code so it can run either the NodeJS or Browser environment
|
||||
|
||||
### Functional(Component) Tests
|
||||
|
||||
✅ DO
|
||||
|
||||
- Write tests for UI components
|
||||
- Write your tests using [Jest](https://jestjs.io/)
|
||||
- Use either Enzyme or React Testing Library to perform component tests.
|
||||
|
||||
### Mocking
|
||||
|
||||
✅ DO
|
||||
|
||||
- Use Jest's built-in mocking helpers
|
||||
|
||||
☑️ YOU SHOULD
|
||||
|
||||
- Write code that does not require mocking
|
||||
- Build components that do not require mocking extremely large or difficult to mock objects (like Explorer.ts). Pass _only_ what you need.
|
||||
|
||||
⛔️ DO NOT
|
||||
|
||||
- Use sinon.js for mocking
|
||||
- Sinon has been deprecated and planned for removal
|
||||
|
||||
### End to End Tests
|
||||
|
||||
✅ DO
|
||||
|
||||
- Use [Puppeteer](https://developers.google.com/web/tools/puppeteer) and [Jest](https://jestjs.io/)
|
||||
- Write or modify an existing E2E test that covers the primary use case of any major feature.
|
||||
- Use caution. Do not try to cover every case. End to End tests can be slow and brittle.
|
||||
|
||||
☑️ YOU SHOULD
|
||||
|
||||
- Write tests that use accessible attributes to perform actions. Role, Title, Label, etc
|
||||
- More information https://testing-library.com/docs/queries/about#priority
|
||||
|
||||
⚠️ YOU SHOULD NOT
|
||||
|
||||
- Add test specfic `data-*` attributes to dom elements
|
||||
- This is a common current practice, but one we would like to avoid in the future
|
||||
- End to end tests need to use semantic HTML and accesible attributes to be truely end to end
|
||||
- No user or screen reader actually navigates an app using `data-*` attributes
|
||||
- Add arbitrary time delays to wait for page to render or element to be ready.
|
||||
- All the time delays add up and slow down testing.
|
||||
- Prefer using the framework's "wait for..." functionality.
|
||||
|
||||
### Migrating Knockout to React
|
||||
|
||||
✅ DO
|
||||
|
||||
- Consult other team members before beginning migration work. There is a significant amount of flux in patterns we are using and it is important we do not propagate incorrect patterns.
|
||||
- Start by converting HTML to JSX: https://magic.reactjs.net/htmltojsx.htm. Add functionality as a second step.
|
||||
|
||||
☑️ YOU SHOULD
|
||||
|
||||
- Write React components that require no dependency on Knockout or observables to trigger rendering.
|
||||
|
||||
## Browser Support
|
||||
|
||||
✅ DO
|
||||
|
||||
- Support all [browsers supported by the Azure Portal](https://docs.microsoft.com/en-us/azure/azure-portal/azure-portal-supported-browsers-devices)
|
||||
- Support IE11
|
||||
- In practice, this should not need to be considered as part of a normal development workflow
|
||||
- Polyfills and transpilation are already provided by our engineering systems.
|
||||
- This requirement will be removed on March 30th, 2021 when Azure drops IE11 support.
|
|
@ -13,6 +13,7 @@ For more information see the [Code of Conduct FAQ](https://opensource.microsoft.
|
|||
contact [opencode@microsoft.com](mailto:opencode@microsoft.com) with any additional questions or comments.
|
||||
|
||||
## Microsoft Open Source Code of Conduct
|
||||
|
||||
This project has adopted the [Microsoft Open Source Code of Conduct](https://opensource.microsoft.com/codeofconduct/).
|
||||
|
||||
Resources:
|
||||
|
@ -20,33 +21,3 @@ Resources:
|
|||
- [Microsoft Open Source Code of Conduct](https://opensource.microsoft.com/codeofconduct/)
|
||||
- [Microsoft Code of Conduct FAQ](https://opensource.microsoft.com/codeofconduct/faq/)
|
||||
- Contact [opencode@microsoft.com](mailto:opencode@microsoft.com) with questions or concerns
|
||||
|
||||
## Browser support
|
||||
Please make sure to support all modern browsers as well as Internet Explorer 11.
|
||||
For IE support, polyfill is preferred over new usage of lodash or underscore. We already polyfill almost everything by importing babel-polyfill at the top of entry points.
|
||||
|
||||
|
||||
## Coding guidelines, conventions and recommendations
|
||||
### Typescript
|
||||
* Follow this [typescript style guide](https://github.com/excelmicro/typescript) which is based on [airbnb's style guide](https://github.com/airbnb/javascript).
|
||||
* Conventions speficic to this project:
|
||||
- Use double-quotes for string
|
||||
- Don't use `null`, use `undefined`
|
||||
- Pascal case for private static readonly fields
|
||||
- Camel case for classnames in markup
|
||||
* Don't use class unless necessary
|
||||
* Code related to notebooks should be dynamically imported so that it is loaded from a separate bundle only if the account is notebook-enabled. There are already top-level notebook components which are dynamically imported and their dependencies can be statically imported from these files.
|
||||
* Prefer using [Fluent UI controls](https://developer.microsoft.com/en-us/fluentui#/controls/web) over creating your own, in order to maintain consistency and support a11y.
|
||||
|
||||
### React
|
||||
* Prefer using React class components over function components and hooks unless you have a simple component and require no nested functions:
|
||||
* Nested functions may be harder to test independently
|
||||
* Switching from function component to class component later mayb be painful
|
||||
|
||||
## Testing
|
||||
Any PR should not decrease testing coverage.
|
||||
|
||||
## Recommended Tools and VS Code extensions
|
||||
* [Bookmarks](https://github.com/alefragnani/vscode-bookmarks)
|
||||
* [Bracket pair colorizer](https://github.com/CoenraadS/Bracket-Pair-Colorizer-2)
|
||||
* [GitHub Pull Requests and Issues](https://github.com/Microsoft/vscode-pull-request-github)
|
Loading…
Reference in New Issue