ToDo items and dependency cleanup #64
Conversation
Sync with primary project repo
There was a problem hiding this comment.
@karnthis thank you for your effort! Please consider some comments.
I haven't got to types refactoring part yet. Will try to do it shortly
| @@ -1,15 +1,9 @@ | |||
| import fetch from 'node-fetch'; | |||
| import { performance } from 'perf_hooks'; | |||
| import { awaitAll, filter, map, prop, sortBy } from '../util'; | |||
| import {ENDPOINT_TYPE, NETWORK_TYPE} from "../types"; | |||
There was a problem hiding this comment.
codestyle seems to be off. Please use prettier and eslint
| import {ENDPOINT_TYPE, NETWORK_TYPE} from "../types"; | |
| import { ENDPOINT_TYPE, NETWORK_TYPE } from "../types"; |
| networkMetadata = res; | ||
| return res; | ||
| }) | ||
| .catch(err => { |
There was a problem hiding this comment.
this catch block seems to be redundant
| if (networkMetadata) { | ||
| return networkMetadata | ||
| } else { | ||
| return fetch(`https://raw.githubusercontent.com/ovrclk/net/master/${network}/meta.json`) |
There was a problem hiding this comment.
It's generally a good idea to await for the promise here. This way if it rejects it's correctly reflected in stacktraces
| return fetch(`https://raw.githubusercontent.com/ovrclk/net/master/${network}/meta.json`) | |
| return await fetch(`https://raw.githubusercontent.com/ovrclk/net/master/${network}/meta.json`) |
| export async function getMetadata(network: NETWORK_TYPE): Promise<INetworkMetadata> { | ||
| return fetch(`https://raw.githubusercontent.com/ovrclk/net/master/${network}/meta.json`) | ||
| .then(res => res.json()); | ||
| if (networkMetadata) { |
There was a problem hiding this comment.
this way to cache works. Although it's really a boilerplate code that ideally would needed to be improved. I'd suggest to add some caching lib for this. The easiest one that would already be nice is lodash with it's memoize fn. Feel free to leave it like this if you like so but if you do, please add a todo based on this comment.
| @@ -1,6 +1,391 @@ | |||
| node_modules | |||
| yarn-error.log | |||
| # Created by https://www.toptal.com/developers/gitignore/api/linux,macos,windows,node,yarn,visualstudiocode,jetbrains,vim | |||
There was a problem hiding this comment.
Not all the things in the generated file make sense. E.g. there's no need to target some specific files in .idea dir, should be ignored all together. If idea adds smth new, it's gonna make it to the codebase.
Also credit to toptal doesn't seem right here. Please remove it.
There was a problem hiding this comment.
why does credit to toptal seem incorrect? that is the tool used to quickly generate a comprehensive .gitignore file.
There was a problem hiding this comment.
I've added some arguments in my 1st comment.
- I believe there are a few incorrect cases.
- Referencing a 3rd party service in our codebase doesn't feel right to me. It's useful ok, however it's not a node_module and it's actually a live file that is maintained by us
| }, | ||
| }; | ||
|
|
||
| /* wrappers */ |
There was a problem hiding this comment.
I'm not sure this comment gives any useful context
There was a problem hiding this comment.
what makes you say that? it is labeling a section of code for clarity. the labeled section is type wrappers that have not changed in form from v2 to v3 but have had their label changed.
There was a problem hiding this comment.
well, I just did not understand why are those wrappers... wrapper is usually something that basically wraps something: like a function calling another function. In here the types are being re-declared, we could call this aliasing.
In anyway, imho comments should be useful. It's pretty obvious what's going on here already. That's why I don't think it should be here. One way I could suggest that would bring sufficient clarity and would not need any comments is to create a dedicated file that's named smth like v3SdlAliases.ts and put all of it there.
ygrishajev
force-pushed
the
main
branch
4 times, most recently
from
87cc746
to
fd3233d
Compare
Addresses the following: