Code Style

[karimStekelenburg]

During the AFJ WG call today we discussed the fact that we have some unwritten rules regarding the coding style in AFJ and related repositories. Because these rules are largely subjective, new contributors have a hard time understanding what these rules are, resulting in a large amount of PR review comments that are purely about code style.

The purpose of this discussion is to gather a list of all the code styling rules we follow, so we can start documenting them. A dedicated section in the documentation site will probably help new contributors get up to speed faster.

[2byrds]

Great stuff. If possible, let's draw attention to this discussion semi-regularly (meetings, reviews, etc) so that it can continue to grow and we can updated the related documentation.

[NB-MikeRichardson]

Ok I have an initial bullet list, these are all things that I needed to learn about and/or tripped me up, causing me to spend time learning the hard way how code should be structured. There are doubtless many more we could (and should) include, but here's my starter...

Third Party Libraries

• Message Validation (class-transformer, class-validator)
• Dependency Injection (tsyringe)
• Testing, Lint Checking (yarn, jest)
• Prettier
• Observables, Callback-based code (rxjs)
• Crypto (mattr)
• Presentation Exchange (DIF pex)

Code Patterns

• Variable names (camel case, conventions)
• Use options interfaces for method parameters
• Check mandatory variables early, throw AriesFrameworkError if falsey
• Include Function parameter and return types
• Put common useful function in utils
• Include tsdoc comments ?
• How we use Generics in AFJ
• type vs interface in TS
• inline vs constants…when and where
• How to avoid circular dependenies (guidance for index.ts and imports)
• Use of Record<string, unknown>
• Use of multiple packages…how they fit together

Some other Minor Rules

• file Naming conventions:
• Use the .ts file extension for TypeScript files
• TypeScript file names should be PascalCase
• Use undefined. Do not use null except where external libraries require it.
• Use === and !== operators whenever possible.
• The == and != operators do type coercion, which is both inefficient and can lead to unexpected behavior.
• Use arrow functions over anonymous function expressions.
• Never use var. Instead use const where possible and otherwise use let.

[2byrds]

Thank you for posting this Mike, looking forward to discussing it tomorrow: https://wiki.hyperledger.org/display/ARIES/2022-08-04+Aries+Framework+JS+Meeting+notes

[berendsliedrecht]

Thats a very good list @NB-MikeRichardson great point to start from.

There are some things that are more specific to AFJ and some things that are more generic TypeScript convention and a seperation between both would be good if we want to turn this into a proper guidelines document.

Couple of minor notes:

• We do not have PascalCase for all files.

I think the current pattern is: use kebab case for any file unless the entire file is a single class (which is the case like 95% of the time). For instance we have index.ts, types.ts and constants.ts. Would be good to have a discussion about a preferred style. Keep it like this, all kebab all pascal, etc.

• We also do use null internally without it coming from a library.
  • https://cs.github.com/hyperledger/aries-framework-javascript/blob/3e415d8327622371961b7d180a9a5ed50d72da51/packages/core/src/modules/oob/domain/OutOfBandEvents.ts#L15
  • https://cs.github.com/hyperledger/aries-framework-javascript/blob/6468a9311c8458615871e1e85ba3f3b560453715/packages/core/src/modules/ledger/IndyPool.ts#L179

Now, the differences between null and undefined can be interpreted as just pure semantics and that is also how we deal with it. For this reason it really becomes an opinionated issue what makes more sense. When we call findFileFromRepository(identifier) and it is not found, do we want to return null or undefined. My preference goes to null 99% of the time. The only reason for undefined, IMO, is something like let foo; // let foo = undefined;. foo here is not a defined value so the variable points to undefined.

I feel like some of this can be enforced with eslint and we should 100000% finish that PR that has been open for months... (we are waiting for the merge of major PRs like ppv2, multitenancy, modularization and issuance of json-ld credentials). I think we should really wait with big features, aftere the before mentioned are merged, and add the rules.

Also, I think should should abstract the type Record<string, unknown away. It does not really explain what it does unless you are aware of the type. Somehting like this would be a good replacement:

type SomeName<Key extends string | number | symbol = string, Value = unknown> = Record<Key, Value>

We have to pick a good name for this, but this type could be documented saying that it just types a nested JSON object.

Type aliasing might be interesting in the framework as well. It might help explaining that the return type of number is actually a WalletHandle. Now, knowing that a WalletHandle is a number can be annoying but its quite an explicit way of telling the return value without using an object.

One last thing is that we should not add typings when they are infered perfectly. Something like this adds a lot of unnecessary text and can make reading the code a bit more cumbersome.

const arr: Array<number> = [1, 2, 3] as const
const num: number = 1;
const x: Record<'foo' | 'bar', number> = {'foo': 1, 'bar': 2} as const

Again, a great start @NB-MikeRichardson I really struggled to even think of 1 thing we should add :).

[2byrds]

Great examples and discussion Berend, i'm already learning a ton :)

[2byrds]

How should we version the docs so that they don't follow only the latest version?