Metamask-extension: Add JSDoc documentation to all background scripts

[danfinlay]

Multiple bugs were shipped last week, in part because interfaces to some files were not clear/understood.

Long term, this looks like a good case for strong types, but in the short term, strong documentation is something our codebase lacks, and could help alleviate this kind of issue.

JSDoc can generate nice readable documentation for a codebase, here is some docs added to our KeyringController (here called mm-vault):
https://github.com/lazaridiscom/mm-vault

Should be added to all scripts in app/scripts, especially app/scripts/controllers and app/scripts/lib.

[frankiebee]

i am working on transaction controller and all it's sub controllers
transactions
tx-state-manager
tx-gas-utils
pending-tx-tracker
nonce-tracker

[danjm]

Here is a checklist with assignees, so that we can track who is working on docs for which file:

• [ ] app/scripts/background.js (Assignee: @danfinlay)
• [ ] app/scripts/chromereload.js (Assignee: DO NOT DOC)
• [ ] app/scripts/config.js (Assignee: @bitpshr)
• [ ] app/scripts/contentscript.js (Assignee: @bitpshr)
• [ ] app/scripts/edge-encryptor.js (Assignee: @bitpshr)
• [ ] app/scripts/first-time-state.js (Assignee: @bitpshr)
• [ ] app/scripts/inpage.js (Assignee: @bitpshr)
• [ ] app/scripts/metamask-controller.js (Assignee: @danfinlay)
• [ ] app/scripts/notice-controller.js (Assignee: @danjm)
• [ ] app/scripts/popup-core.js (Assignee: @bitpshr)
• [ ] app/scripts/ui.js (Assignee: @danjm)
• [ ] app/scripts/controllers/address-book.js (Assignee: @danjm)
• [ ] app/scripts/controllers/balance.js (Assignee: @danjm)
• [ ] app/scripts/controllers/blacklist.js (Assignee: @danjm)
• [ ] app/scripts/controllers/computed-balances.js (Assignee: @bitpshr)
• [ ] app/scripts/controllers/currency.js (Assignee: @danjm)
• [ ] app/scripts/controllers/infura.js (Assignee: @danjm)
• [ ] app/scripts/controllers/network.js (Assignee: @danjm)
• [ ] app/scripts/controllers/preferences.js (Assignee: @danjm)
• [ ] app/scripts/controllers/recent-blocks.js (Assignee: @danjm)
• [ ] app/scripts/controllers/shapeshift.js (Assignee: @danjm)
• [ ] app/scripts/controllers/transactions.js (Assignee: @frankiebee)
• [ ] app/scripts/lib/account-tracker.js (Assignee: @danjm)
• [ ] app/scripts/lib/auto-reload.js (Assignee: @danjm)
• [ ] app/scripts/lib/buy-eth-url.js (Assignee: @danjm)
• [ ] app/scripts/lib/config-manager.js (Assignee: @danjm)
• [ ] app/scripts/lib/createLoggerMiddleware.js (Assignee: @whymarrh)
• [ ] app/scripts/lib/createOriginMiddleware.js (Assignee: @whymarrh)
• [ ] app/scripts/lib/createProviderMiddleware.js (Assignee: @bitpshr)
• [ ] app/scripts/lib/environment-type.js (Assignee: @danjm)
• [ ] app/scripts/lib/events-proxy.js (Assignee: @whymarrh)
• [ ] app/scripts/lib/extractEthjsErrorMessage.js (Assignee: @danjm)
• [ ] app/scripts/lib/get-first-preferred-lang-code.js (Assignee: @danjm)
• [ ] app/scripts/lib/getObjStructure.js (Assignee: @danjm)
• [ ] app/scripts/lib/hex-to-bn.js (Assignee: @whymarrh)
• [ ] app/scripts/lib/inpage-provider.js (Assignee: @danjm)
• [ ] app/scripts/lib/is-popup-or-notification.js (Assignee: @danjm)
• [ ] app/scripts/lib/local-store.js (Assignee: @whymarrh)
• [ ] app/scripts/lib/message-manager.js (Assignee: @danjm)
• [ ] app/scripts/lib/nodeify.js (Assignee: @danjm)
• [ ] app/scripts/lib/nonce-tracker.js (Assignee: @frankiebee)
• [ ] app/scripts/lib/notification-manager.js (Assignee: @danjm)
• [ ] app/scripts/lib/pending-balance-calculator.js (Assignee: @danjm)
• [ ] app/scripts/lib/pending-tx-tracker.js (Assignee: @frankiebee)
• [ ] app/scripts/lib/personal-message-manager.js (Assignee: @danjm)
• [ ] app/scripts/lib/port-stream.js (Assignee: @bitpshr)
• [ ] app/scripts/lib/random-id.js (Assignee: @whymarrh)
• [ ] app/scripts/lib/reportFailedTxToSentry.js (Assignee: @danjm)
• [ ] app/scripts/lib/seed-phrase-verifier.js (Assignee: @danjm)
• [ ] app/scripts/lib/setupMetamaskMeshMetrics.js (Assignee: @bitpshr)
• [ ] app/scripts/lib/setupRaven.js (Assignee: @danjm)
• [ ] app/scripts/lib/stream-utils.js (Assignee: @whymarrh)
• [ ] app/scripts/lib/tx-gas-utils.js (Assignee: @frankiebee)
• [ ] app/scripts/lib/tx-state-history-helper.js (Assignee: @frankiebee)
• [ ] app/scripts/lib/tx-state-manager.js (Assignee: @frankiebee)
• [ ] app/scripts/lib/typed-message-manager.js (Assignee: @danjm)
• [ ] app/scripts/lib/util.js (Assignee: @danjm)
• [ ] app/scripts/platforms/extension.js (Assignee: @danfinlay)
• [ ] app/scripts/platforms/sw.js (Assignee: @whymarrh)
• [ ] app/scripts/platforms/window.js (Assignee: @whymarrh)

[danjm]

For now, I will take the following:
app/scripts/notice-controller.js
app/scripts/ui.js
app/scripts/controllers/address-book.js
app/scripts/controllers/currency.js
app/scripts/controllers/preferences.js
app/scripts/controllers/shapeshift.js
app/scripts/lib/buy-eth-url.js
app/scripts/lib/environment-type.js
app/scripts/lib/get-first-preferred-lang-code.js
app/scripts/lib/is-popup-or-notification.js
app/scripts/lib/nodeify.js
app/scripts/lib/util.js

[whymarrh]

Something to keep in mind, if we're interested, we can use the TypeScript type signatures in the docblocks we add (e.g. in @params) and run the TypeScript compiler against them file by file. For example, in #3984 I was working locally with this config:

{
    "compileOnSave": true,
    "compilerOptions": {
        "allowJs": true,
        "alwaysStrict": true,
        "allowSyntheticDefaultImports": true,
        "checkJs": true,
        "jsx": "preserve",
        "lib": [
            "dom",
            "dom.iterable",
            "es7"
        ],
        "moduleResolution": "node",
        "noEmit": true,
        "noFallthroughCasesInSwitch": true,
        "noImplicitAny": true,
        "noImplicitReturns": true,
        "noImplicitThis": true,
        "noUnusedLocals": true,
        "noUnusedParameters": true,
        "strictNullChecks": true,
        "target": "es6",
        "typeRoots": [
            "node_modules/@types/",
            "types/"
        ]
    },
    "include": [
        "app/scripts/lib/migrator/*",
        "app/scripts/lib/createLoggerMiddleware.js",
        "app/scripts/lib/createOriginMiddleware.js",
        "app/scripts/lib/events-proxy.js",
        "app/scripts/lib/hex-to-bn.js",
        "app/scripts/lib/local-store.js",
        "app/scripts/lib/random-id.js",
        "app/scripts/lib/stream-utils.js",
        "app/scripts/platforms/sw.js",
        "app/scripts/platforms/window.js"
    ]
}

The process for running TypeScript against the docs we write would be:

1. Adding the TypeScript package
2. Adding a tsconfig.json (as shown above) to the project root
3. Running tsc --project . to check the files

I both think that this is just a useful approach locally when writing JSDocs to make sure that I've covered everything, as well as a desirable check to add to CI (though I won't necessarily push for that).

@bitpshr might have thoughts on this option and whether or not it's a good idea/useful. There are a lot of Salsa bugs but it does work for enough cases that I think it's worthwhile.

[danfinlay]

That's an interesting idea, @whymarrh, I'd be open to trying that out.

[danjm]

Files which no one is yet assigned to document:

• [ ] app/scripts/background.js (Assignee: )
• [ ] app/scripts/chromereload.js (Assignee: ) (Not sure that we need to document this... looks like it is copied from somewhere else?)

[bitpshr]

I will start with the following:

app/scripts/config.js
app/scripts/contentscript.js
app/scripts/edge-encryptor.js
app/scripts/first-time-state.js
app/scripts/inpage.js
app/scripts/popup-core.js

[danjm]

I'll take these:

• [ ] app/scripts/controllers/infura.js (Assignee: @danjm)
• [ ] app/scripts/controllers/network.js (Assignee: @danjm)
• [ ] app/scripts/lib/config-manager.js (Assignee: @danjm)
• [ ] app/scripts/lib/inpage-provider.js (Assignee: @danjm)
• [ ] app/scripts/lib/reportFailedTxToSentry.js (Assignee: @danjm)
• [ ] app/scripts/lib/setupRaven.js (Assignee: @danjm)

[bitpshr]

I will take these:

app/scripts/controllers/computed-balances.js
app/scripts/lib/createProviderMiddleware.js
app/scripts/lib/port-stream.js
app/scripts/lib/setupMetamaskMeshMetrics.js

[bdresser]

Team is this still in play? Looks like a bunch of pieces have been merged, any outstanding?

[whymarrh]

I _think_ everything in Dan's comment above https://github.com/MetaMask/metamask-extension/issues/3941#issuecomment-380497049 has been done

[bdresser]

nice! are we exporting the summary documentation? if not, we should 😄