Add initial guidelines for JavaScript and TypeScript #11

mcmire · 2022-10-11T23:27:01Z

These were copied from the notes that I've been keeping in Notion (see "Code Guidelines"). I tried to list best practices that I thought were already being followed in some form, so hopefully none of them should be too controversial.

Fixes #10.

👉🏻 See rendered JavaScript guidelines 👈🏻
👉🏻 See rendered TypeScript guidelines 👈🏻

guides/javascript.md

guides/typescript.md

These were copied from the notes that I've been keeping in Notion (see "Code Guidelines"). I tried to list best practices that I thought were already being followed in some form, so hopefully none of them should be too controversial.

Mrtenz · 2023-07-21T21:58:43Z

guides/typescript.md

+
+These guidelines specifically apply to TypeScript.
+
+## Provide explicit return types for functions/methods


This can be enforced with ESLint.

Good point! I've added the explicit-function-return-type rule here: MetaMask/eslint-config#314

By the way, given that we have some ESLint rules that we've enabled ourselves, what do we feel about explicitly documenting the reason for doing so? That means we'd keep this here but then have something at the top that says something like:

> **Note** > This guideline is enforced via [`explicit-function-return-type`](/link/to/rule/in/our/eslint-config).

That seems like a good idea to me. We can remove it later if we don't think it has been useful.

Generally it would be nice to keep this guide shorter, and let ESLint explain things for us where possible. But the ESLint rules don't explain our reasoning very well. I wish we could.... explain the reasons for lint rules inline and advise a course of action, something like that. Ah well.

Agreed... okay. I can add this in a followup PR once we have a new version of eslint-config out.

guides/javascript.md

Gudahtt · 2023-07-26T00:00:38Z

guides/typescript.md

Things that would be nice to have guidelines for:

prefer ts-expect-error over ts-ignore

When should we use ts-expect-error?

When is it OK to use any?

How to effectively narrow types (particularly when dealing with unknown).

Related: how to write type guard functions

No unnecessary generic parameters

Okay! I will add these in a followup PR.

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Gudahtt · 2023-07-26T20:51:36Z

guides/testing.md

+* `jest.spyOn(object, method)` can be used in place of `sinon.stub(object, method)` (with the caveat that the method being spied upon will still be called by default).
+* `jest.useFakeTimers()` can be used in place of `sinon.useFakeTimers()` (though note that Jest's "clock" object had fewer features than Sinon's prior to Jest v29.5).
+
+## Don't test private code


It would be nice to expand more on the reasoning behind this.
e.g.

Explain how this ties in with a broader BDD-like testing philosophy,

Tests are more useful for preventing regressions and helping document intended behavior when they only reflect the public API

This ensures the tests help make refactoring easier, rather than getting in the way

This is a great start though! I can follow up to add more detail later.

Sounds good!

guides/javascript.md

Gudahtt

LGTM!

guides/javascript.md

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

brad-decker

⭐ 💯

mcmire force-pushed the add-js-and-ts-guidelines branch from 2fbc8e3 to 31f6326 Compare October 12, 2022 21:33

mcmire changed the title ~~WIP: Add guides for JavaScript and TypeScript~~ Add guides for JavaScript and TypeScript Oct 12, 2022

mcmire force-pushed the add-js-and-ts-guidelines branch from 31f6326 to b5d16de Compare October 12, 2022 21:35

mcmire changed the title ~~Add guides for JavaScript and TypeScript~~ Add code guidelines for JavaScript and TypeScript Oct 12, 2022

mcmire commented Oct 12, 2022

View reviewed changes

guides/javascript.md Outdated Show resolved Hide resolved

mcmire marked this pull request as ready for review October 12, 2022 21:36

brad-decker reviewed Oct 14, 2022

View reviewed changes

guides/javascript.md Outdated Show resolved Hide resolved

mcmire commented Nov 9, 2022

View reviewed changes

guides/typescript.md Outdated Show resolved Hide resolved

mcmire force-pushed the add-js-and-ts-guidelines branch from 6b3a793 to 6ec68d0 Compare November 14, 2022 16:18

mcmire changed the base branch from main to assign-guideline-categories November 14, 2022 16:20

Base automatically changed from assign-guideline-categories to main December 1, 2022 21:23

mcmire marked this pull request as draft July 19, 2023 17:51

mcmire changed the base branch from main to update-guidelines-for-guidelines July 19, 2023 19:49

mcmire force-pushed the add-js-and-ts-guidelines branch from 2a5ddf3 to dffe1dd Compare July 19, 2023 19:50

mcmire force-pushed the update-guidelines-for-guidelines branch 2 times, most recently from 72aadda to 7269c7c Compare July 19, 2023 23:23

mcmire force-pushed the add-js-and-ts-guidelines branch from 9f8db81 to 27219ab Compare July 19, 2023 23:35

Base automatically changed from update-guidelines-for-guidelines to main July 21, 2023 04:29

mcmire force-pushed the add-js-and-ts-guidelines branch from 27219ab to 455be22 Compare July 21, 2023 21:43

Add initial code guidelines for JS/TS

acbcc47

These were copied from the notes that I've been keeping in Notion (see "Code Guidelines"). I tried to list best practices that I thought were already being followed in some form, so hopefully none of them should be too controversial.

mcmire force-pushed the add-js-and-ts-guidelines branch from 455be22 to acbcc47 Compare July 21, 2023 21:44

mcmire changed the title ~~Add code guidelines for JavaScript and TypeScript~~ Add initial guidelines for JavaScript and TypeScript Jul 21, 2023

mcmire marked this pull request as ready for review July 21, 2023 21:45

Mrtenz reviewed Jul 21, 2023

View reviewed changes

mcmire mentioned this pull request Jul 25, 2023

Enforce specification of return types in TypeScript MetaMask/eslint-config#314

Merged

Gudahtt reviewed Jul 25, 2023

View reviewed changes

guides/javascript.md Outdated Show resolved Hide resolved

mcmire added 2 commits July 25, 2023 17:21

Add explanation and disclaimer for 'await promises' guideline

ef266d4

Remove 'Read more' section here since we already linked to it

d632a27

Gudahtt reviewed Jul 25, 2023

View reviewed changes

guides/javascript.md Outdated Show resolved Hide resolved

Gudahtt reviewed Jul 25, 2023

View reviewed changes

guides/javascript.md Show resolved Hide resolved

Gudahtt reviewed Jul 25, 2023

View reviewed changes

guides/javascript.md Outdated Show resolved Hide resolved

Gudahtt reviewed Jul 25, 2023

View reviewed changes

guides/javascript.md Outdated Show resolved Hide resolved

Gudahtt reviewed Jul 26, 2023

View reviewed changes

mcmire and others added 6 commits July 26, 2023 14:18

Update wording for 'Use a verb phrase' guideline

6d93d07

Update "Place names of secondary concepts first" guideline

a751e33

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Update examples in 'Place names of secondary concepts first'

7b8e6ae

Update point in "Use Jest's mock functions" guideline

2082d92

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Extract testing.md

aa8fa2f

Remove disclaimer about performance of awaiting promises

22a9daa

Gudahtt reviewed Jul 26, 2023

View reviewed changes

guides/javascript.md Outdated Show resolved Hide resolved

Gudahtt previously approved these changes Jul 26, 2023

View reviewed changes

Mrtenz requested changes Jul 27, 2023

View reviewed changes

guides/javascript.md Outdated Show resolved Hide resolved

guides/javascript.md Outdated Show resolved Hide resolved

Use single quotes

982de23

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

mcmire dismissed Gudahtt’s stale review via 982de23 July 27, 2023 14:46

mcmire and others added 2 commits July 27, 2023 08:46

Use single quotes

2795422

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

Fix wording

59538fc

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Gudahtt approved these changes Jul 27, 2023

View reviewed changes

Mrtenz approved these changes Jul 27, 2023

View reviewed changes

brad-decker approved these changes Jul 27, 2023

View reviewed changes

mcmire merged commit b485485 into main Jul 27, 2023

mcmire deleted the add-js-and-ts-guidelines branch July 27, 2023 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial guidelines for JavaScript and TypeScript #11

Add initial guidelines for JavaScript and TypeScript #11

mcmire commented Oct 11, 2022 •

edited

Loading

Mrtenz Jul 21, 2023

mcmire Jul 25, 2023

mcmire Jul 25, 2023

Gudahtt Jul 26, 2023 •

edited

Loading

mcmire Jul 26, 2023

Gudahtt Jul 26, 2023 •

edited

Loading

mcmire Jul 26, 2023

Gudahtt Jul 26, 2023

mcmire Jul 26, 2023

Gudahtt left a comment

brad-decker left a comment


		These guidelines specifically apply to TypeScript.

		## Provide explicit return types for functions/methods

Add initial guidelines for JavaScript and TypeScript #11

Add initial guidelines for JavaScript and TypeScript #11

Conversation

mcmire commented Oct 11, 2022 • edited Loading

Mrtenz Jul 21, 2023

Choose a reason for hiding this comment

mcmire Jul 25, 2023

Choose a reason for hiding this comment

mcmire Jul 25, 2023

Choose a reason for hiding this comment

Gudahtt Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

mcmire Jul 26, 2023

Choose a reason for hiding this comment

Gudahtt Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

mcmire Jul 26, 2023

Choose a reason for hiding this comment

Gudahtt Jul 26, 2023

Choose a reason for hiding this comment

mcmire Jul 26, 2023

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

brad-decker left a comment

Choose a reason for hiding this comment

mcmire commented Oct 11, 2022 •

edited

Loading

Gudahtt Jul 26, 2023 •

edited

Loading

Gudahtt Jul 26, 2023 •

edited

Loading