TypeScript migration #174
TypeScript migration #174
Conversation
| `${apiUrl}/api?module=account&action=txlist&address=${address}&tag=latest&page=1&offset=1`, | ||
| ); | ||
| const parsedResponse = await response.json(); | ||
| if (parsedResponse.status !== '0' && parsedResponse.result.length > 0) { |
There was a problem hiding this comment.
Nit: Again, maybe out of the scope of the MR, but this if statement is a bit verbose. It could be safely refactored to something that, IMO, is more readable, like this:
const parsedRes = await response.json();
const resExists = parsedRes.status !== '0' && parsedRes.result?.length;
return resExists;At the very least, it could just be shortened to
return parsedResponse.status !== '0' && parsedResponse.result.length > 0|
|
||
| hdPath = hdPathString; | ||
|
|
||
| paths: Record<string, number> = {}; |
There was a problem hiding this comment.
These Record's for this.paths and this.accountDetails could be made readonly via Readonly<Record<...>>, the only thing it has very slight chance of breakage depending on how the app is handled. Maybe best to allow these objects to be mutable
src/ledger-bridge-keyring.ts
Outdated
| return tmp.join('/'); | ||
| } | ||
|
|
||
| private _sendMessage( |
There was a problem hiding this comment.
this should be changed to sharp notation too, but at this moment most of the tests spy this method
(See also #174 (comment))
There was a problem hiding this comment.
we can spy and stub the iframe content window postMessage method and call the respective callback, instead of stubbing sendMessage, taking for granted that whenever postMessage is called, a callback fn has been added to keyring.messageCallbacks, so we can manually call it passing the desired iframe message response:
sandbox.on(
keyring?.iframe?.contentWindow as Window,
'postMessage',
(msg) => {
keyring.messageCallbacks[msg.messageId]?.({
...msg,
success: true,
payload: { foo: 'bar' },
});
},
);
There was a problem hiding this comment.
I skimmed over the JavaScript versions of ledger-bridge-keyring and the test file and compared with the TypeScript files and didn't see any glaring differences, but I'll do a more in-depth check later. In the meantime I've left some comments around the TypeScript implementation.
| '0x6772C4B1E841b295960Bb4662dceD9bb71726357', | ||
| '0x41bEAD6585eCA6c79B553Ca136f0DFA78A006899', | ||
| '0xf37559520757223264ee707d4e3fdfaa118db9bd', | ||
| ] as const; |
There was a problem hiding this comment.
Nit: Do we need this array to be an exact length, and/or is a function expecting an argument with a type of, say, '0xf37559520757223264ee707d4e3fdfaa118db9bd'? If not, it seems like this wouldn't need to be as const.
There was a problem hiding this comment.
We can remove as const but then the type of fakeAccounts would be Array<string | undefined>, so we'll have to use an optional chain or a cast on each test where we use those, even if we know they are valid
.eslintrc.js
Outdated
| }, | ||
| ], | ||
|
|
||
| rules: { | ||
| 'import/no-nodejs-modules': 'off', | ||
| 'no-restricted-globals': [ |
There was a problem hiding this comment.
Hmm. It's too bad that there's not another way to do this. That said, nice job for reusing the list already defined in @metamask/eslint-config — we should use that technique more often.
@Gudahtt Do you have any suggestions for how we could make this better?
There was a problem hiding this comment.
We could use this package, if we want to require this only be used in-browser: https://github.com/MetaMask/eslint-config/tree/main/packages/browser
If we're anticipating this will be used in a Node.js context, with polyfills for each referenced browser global, than this approach is probably best. The alternative would be to eslint-ignore each individual violation (though maybe we could do that in a nice way by bringing them all to the top of the module and reassigning them to variables).
There was a problem hiding this comment.
Perhaps using the browser lint package here would be a good idea. This keyring does assume a browser context.
There was a problem hiding this comment.
In some lines, we use the Buffer Node.js built-in without using any particular import to handle that, and indeed after using @metamask/eslint-config-browser those Buffer.from(...) are marked as error.
Adding import { Buffer } from 'buffer'; should do the trick but I'm unsure how it worked up until now. Is that just a TS requirement in your opinion?
There was a problem hiding this comment.
Hmm, great question. I would have expected it to fail prior to adding the browser config.
There was a problem hiding this comment.
Oh sorry, I misunderstood. So that global is available in Node.js and injected by the extension build system, that's why it had worked before. It would be ideal to remove that from this package, but allowing that global as an exception or importing it directly should work instead for now (either should be equivalent)
There was a problem hiding this comment.
This is a good example of why this browser ESLint package is useful; it can highlight undocumented requirements like this. Now we know that this package assumes there is a Buffer polyfill present. We could document that requirement in the README so third-parties know about that requirement.
There was a problem hiding this comment.
Not sure it is the best approach: I suppose that we are still in a Node environment when running unit tests, so @metamask/eslint-config-browser is used just on *.ts files, and I added @metamask/eslint-config-nodejs to *.test.ts files.
Additionally, as in tests we still need a couple of types from the DOM, I assigned those types to other variables on top of the file and eslint-ignored them: we are able to use those types as we set DOM as typescript build target
mikesposito
force-pushed
the
feat/typescript-migration
branch
from
e232f31 to
fe77538
Compare
This PR converts the package from JavaScript to TypeScript, adding needed devDependencies and configurations to make it work. The linting pipeline has also been updated, bringing some changes to the code to make it compliant with the latest module template version.
typedocdepcheckand relatedpackage.jsonscriptspadLeftEven,normalize,toAsciias unusedFixes #23
Fixes #164