diff --git a/CODING_GUIDELINES.md b/CODING_GUIDELINES.md new file mode 100644 index 000000000..daa14a470 --- /dev/null +++ b/CODING_GUIDELINES.md @@ -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. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bee66a903..59386e0b8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,6 +1,6 @@ # Contribution guidelines to Data Explorer -This project welcomes contributions and suggestions. Most contributions require you to agree to a +This project welcomes contributions and suggestions. Most contributions require you to agree to a Contributor License Agreement (CLA) declaring that you have the right to, and actually do, grant us the rights to use your contribution. For details, visit https://cla.microsoft.com. @@ -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) \ No newline at end of file