search

MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
11.88k stars 4.85k forks source link

Add JSDoc documentation to all background scripts #3941

Closed danfinlay closed 4 years ago

danfinlay commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

danjm commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

danjm commented 6 years ago

Files which no one is yet assigned to document:

bitpshr commented 6 years ago

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 commented 6 years ago

I'll take these:

bitpshr commented 6 years ago

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 commented 6 years ago

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

whymarrh commented 6 years ago

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

bdresser commented 6 years ago

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